consider a move to Bootstrap 4

[lddubeau]

Packages we depend on, not yet converted to BS 4:

• bootbox
• bootstrap-notify

(corejs-typeahead already has a style for BS 4.)

It also means:

• converting our custom LESS code to SCSS.
• restyling all our widgets
• removing patches/workarounds for BS 3
• introducing new patches/workarounds for BS 4???

It constitutes a breaking change.

[lddubeau]

Upgrading would also stop github from complaining about this. However, per the Debian Security Team, the version we are using is not affected.

[tiesont]

Can't promise it's bug-free, but as a starting point, I've updated my fork (https://github.com/tiesont/bootbox) of Bootbox to be both BS3 and BS4 compatible and add a few features. I'm a collaborator for the original repo, but that doesn't give me sufficient rights to make some changes I want/need to make to push an updated version, so that's the best I can offer. HTH.

[lddubeau]

@tiesont Thanks. After my comment, I did update my dependencies to use your code. I needed that to at least get me over the immediate issue that I could not even make sure my code works fine with BS4. (This updated code has not been pushed yet.)

The lack of an actual npm bootbox package supporting BS4 is still a stumbling block though. There currently are a few dependencies in the package.json of my project that point to git repos but I'm aiming to move away from such dependencies. Dependencies on git repos cause a slew of problems when it comes to managing dependencies. For instance, there's no standard git URL equivalent to the dependency foo: "^1" or foo: ">=2 <5" Some projects work around it by managing git branches or tags to provide some of the missing functionality. So a project could set the branch v1.x.x to simulate semver ^1, but there's no established standard these workarounds so it is up to the developer to discover whether the specific project they are using has any such support. And it still does not support more complex cases. And because there's no standard way to support such ranges, 3rd party tooling typically cannot use it. Ultimately, dependencies with git URLs need babysitting, and I'm aiming to move away from that. I've been progressively turning on dependency management tools on all my repos. (Like Greenkeeper, but I'm moving to Renovate, which I find much much better than Greenkeeper.)

I'm getting close to just doing myself what I suggested to you. I'd fork bootbox, give a new name to the project, merge your code with the main branch, edit the README to explain the history of the project, and produce an npm package under a new name. I'd much rather let someone who's more invested into the project's code do this, but I don't see the current status quo being viable.

[tiesont]

@lddubeau Thank you for the reply.

I'll put some thought into creating a wholly separate project based on bootbox. I mostly maintain it simply because I still actively use it in projects, and it's preferable to the "normal" way of using BS modals.

My primary concern is that I'm mostly a .NET guy - I know JavaScript well enough to be dangerous, but I'm by no means an expert.

[lddubeau]

@tiesont Alright, I'm working on a fork. There's something I don't understand. There's src/bootbox.locales.js and there are the files in src/locales/. What's the relationship between the files that contain one language (in src/locales/) and the one that contains all languages (src/bootbox.locales.js)?

By the way, there were roughly 50 tests that did not run when running the test suite in the v5.x branch. Once I fixed the suite to get these tests to run, I found bugs which I've fixed in my fork. (Not public yet.)

[tiesont]

The intent was that the bootbox.locales.js file would be built from the individual locale files. That would (in theory) make it easier to either commit a new locale or update an existing - you wouldn't have to deal with the whole file. I wasn't really able to get a build script working, though, so to make it (bootbox) work the way I intended I just hand-built the bootbox.locale.js file.

The idea was that most people aren't using most of the locales, so bootbox.js contains just the en locale to save a handful of bits.

The tests should have been running, unless I missed something, but then again, it's been a bit since I was really working on 5.x. Then again, it's possible that I just didn't notice that they weren't being applied. I just go by what Travis-CI tells me: https://travis-ci.org/tiesont/bootbox

[lddubeau]

@tiesont Thanks for the clarification. I'll set something to avoid having to edit multiple files if changes need to be made to existing languages or adding a new language.

Travis is telling you the number of tests that Mocha has discovered, but the problem happens at the time Mocha tries to discover tests. I did not add any tests myself but I get over 500 tests here when I run the suite.

Here's the issue. Originally the tests were written in CoffeeScript. Then they were converted to JS and continued being edited in JS. In CoffeeScript the test suite was something like this:

describe "some tests", ->
  describe "about something", ->
    it "a", ->
    it "b", ->
  describe "about something else", ->
    it "c", ->
    it "d", ->

In CoffeeScript, the last expression of a function is the return value of the function, so the code above is converted to this:

describe("some tests", function() {
  describe("about something", function() {
    it("a", function() {});
    return it("b", function() {});
  });
  return describe("about something else", function() {
    it("c", function() {});
    return it("d", function() {});
  });
});

Note all the return statements. They serve no purpose, but they are valid JavaScript, and Mocha does not use the value returned from a describe callback, so it does not care. The code above is not wrong, it just contains pointless return statements. So far so good, but then someone decides to drop the CoffeeScript code so all the tests are permanently converted to JavaScript, and then someone decides to add more tests and edits the JavaScript code to this:

describe("some tests", function() {
  describe("about something", function() {
    it("a", function() {});
    return it("b", function() {});
  });
  return describe("about something else", function() {
    it("c", function() {});
    return it("d", function() {});
  });

  // Here are additional tests added manually after the file was converted to JS.
  describe("some more tests", ....);
});

Unfortunately, the new tests won't be run. Why? Because the JavaScript interpreter does not even get to execute the last describe that was manually added. Look at where the return statements are. The function that contains the new describe returns before it gets to the new describe. The interpreter has no issue, because it is valid JavaScript code (though probably not doing what the developer wants). Mocha does not complain because it does not know about the describe which is missed. And Travis just runs Mocha.

The only way to catch this is by inspecting the code or running a correctly configured linter on the test code. (Any linter worth the name would report the unreachable code after the return.)

I happened to find the problem because one of the first things I did with the test files was to manually remove all the CoffeeScript-isms from them. There was also an overarching problem because the tests were storing data into this, which is a really really bad idea. Unfortunately the developers of Mocha produced some early examples that used this to store test data, and some folks got the idea from there, but Mocha's developers later discouraged the practice. The upshot in this project is that as soon as I upgraded Mocha to a recent version, the test suite crashed hard because it used this incorrectly. I rewrote the code to avoid the use of this and since the change was so extensive, I also removed the CoffeeScript-isms at the same time. Otherwise, the missed tests would have gone unnoticed for a while, until I setup a correctly configured linter. (I ran grunt and saw a bunch of jshint errors but at the moment it is misconfigured, and I plan to replace it with something much better anyway (eslint).)

[tiesont]

Thank you for the explanation. If you get to a point where you could use some help, let me know, even if it's just help writing/updating documentation.

[tiesont]

I assume I mentioned this somewhere already, but I was able to get access to the npm package for Bootbox, so if you're still using it in this project instead of bootprompt, Bootbox is now Bootstrap 4 compatible.

Just FYI.

[lddubeau]

Yep, I saw the news. All the projects I have that were using bootbox are now using bootprompt.