Error with auto-traced jQuery and Bootstrap v3

[chabou-san]

I'm submitting a bug report

• Library Version:
  1.0.0-beta.3

Please tell us about your environment:

• Operating System:
  OSX 10.x

• Node Version:
  8.12.0

• NPM Version:
  yarn 1.10.1

• Browser:
  Chrome 70 | Firefox 63.0 | Safari 12.0

• Language:
  ESNext

• Loader/bundler:
  RequireJS

Current behavior:
I've just upgraded an old app of mine from Aurelia-cli v0.33.0 to 1.0.0-beta.3. Due to the major changes, I tried to use the recipes described in the cookbook for both jQuery and Bootstrap v3.
The recipe for jQuery basically says it can be auto-traced (so I removed it from my aurelia.json).
The recipe for bootstrap v3 says it shall be manually configured in aurelia.json (so I updated my conf to remove the CSS resource I have previously added, but that was it).
When using both recipes, and since bootstrap depends on jQuery, my app is bundled without warning, but when I execute it, I got an error where Bootstrap says it requires jQuery but can't find it, whereas au-cli has the (expected) following trace:

INFO [Bundler] Auto tracing package: 3.3.1      jquery

When I add the jQuery dependency back into my aurelia.json, everything works as expected.

Since jQuery is auto-traced, it is probably added (in the vendor-bundle.js file) AFTER the manually-traced Bootstrap v3 library, even though bootstrap explicitly depends on jQuery. I guess that when jQuery is manually-traced, dependency resolution succeeds in placing jQuery before Bootstrap v3 and thus it explains why my app works.

• What is the expected behavior?
  I expect my application not to crash when I follow the recipes. :-)

• What is the motivation / use case for changing the behavior?
  I want the conjunction of the two recipes to work (but I'm not sure it is even possible without a major rewrite), or at least have the legacy bootstrap's recipe corrected to reflect the incompatibility of the two recipes.

[3cp]

Following the cookbook, are you sure you have "deps":["jquery"] for bootstrap in aurelia.json?

That deps (shim) should enforce the order of jquery and bootstrap.

[chabou-san]

Yes, this is the Bootstrap v3 configuration I have :

          {
            "name": "bootstrap",
            "path": "../node_modules/bootstrap/dist",
            "main": "js/bootstrap.min",
            "deps": ["jquery"]
          },

The test I make to reproduce the issue is adding/removing the "jquery" (raw, just that string) dependency from the dependencies array in my aurelia.json.
I can definitely confirm that jQuery really is placed (in my vendor-bundle.js) after all my manually added packages (even though they all depends on jQuery):

INFO [Bundle] Manually adding package: 3.3.7      bootstrap
INFO [Bundle] Manually adding package: 2.5.2      bootstrap-colorpicker
INFO [Bundle] Manually adding package: 0.4.0      gridstack
INFO [Bundle] Manually adding package: 1.12.1     jquery-ui
INFO [Bundle] Manually adding package: 0.11.1     bloodhound
INFO [Bundle] Manually adding package: 0.11.1     typeahead.js

[...]

INFO [Bundler] Auto tracing package: 3.3.1      jquery

[3cp]

There must be some regression in latest beta. But I can not do anything now because of vacation. I will come back to you on this bug. Sorry for the inconvenience.

[3cp]

@chabou-san could you try "aurelia-cli": "huochunpeng/cli#fix-shim-order", "aurelia-cli": "^1.0.0-beta.5", in your app's package.json (plus re-run npm install`)?

It should fix the Bootstrap v3 problem, let me know if it works. Thx!

[3cp]

1.0.0-beta.5 released, pls test against it.

[chabou-san]

Just tested it, and it's not fixed sorry.
So I've taken a quick look at your PR and the related code, and here's something that might be helpful:
I've logged the array from getBundledFiles and my jQuery dependency is listed as jquery/dist/jquery.
But when the (double) sort is done, dependencies are based on what's listed in aurelia.json. And in my case, for bootstrap, the dependency is jquery. Therefore the following sort function (the one based on dependencies) doesn't work as expected since "jquery" <> "jquery/dist/jquery".

Am I making myself clear ?

[3cp]

The sort is based on package name jquery, not module name jquery/dist/jquery.

Can you sent me your generated vendor-bundle.js in gitter or discourse? I need to see what’s happening inside. Thx!

[chabou-san]

✅ Done on Aurelia-discourse.

The sort is based on package name jquery, not module name jquery/dist/jquery.

Ah, ok. But I can tell that the array bundleFiles still contains jquery after bootstrap.
Not sure if that helps, but I could provide you with my full list of dependencies, or custom steps I could execute to help you.
Also, custom logs I added seem to show that in the shim sorting function, jquery is never tested against bootstrap. 😮 They are just both called a unique time, against another dependency (which is @fortawesome/fontawesome-free-solid FTR)

Edit: the more I think of it, the more I'm convinced that it comes from the use of a sort function where 0 is returned when 2 modules are not related. Due to the comparison of @fortawesome/fontawesome-free-solid vs bootstrap (returns 0) and @fortawesome/fontawesome-free-solid vs jquery (returns 0), it shall think that jquery and bootstrap are "equal" and thus doesn't need to be compared, thus they're not sorted.

[3cp]

Thx for the file vendor-bundle, you can remove the gist now. I can reproduce the issue.

Working on it, the sorting is still unstable, because not all combination of two packages were checked by the sorting.

[3cp]

The bug can be demonstrated by this:

> function s(a, b) {
// for example, we want to enforce "a" to be after "c"
// but "a" "c" combination check could be skipped
... if (a === 'a' && b === 'c') return 1;
... if (a === 'c' && b === 'a') return -1;
... return 0;
... }
undefined
> ['a', 'b', 'c'].sort(s);
// the sorting algorithm checked "a" "b" and "b" "c" combination, and as far as it concerns, it's sorted.
[ 'a', 'b', 'c' ] // wrong! "a" should be after "c"
> ['a', 'c', 'b'].sort(s);
// this one checked "a" "c" combination
[ 'c', 'a', 'b' ]
> 

[3cp]

@fkleuver can I borrow you for few minutes? How to make the above code stable?

[fkleuver]

@huochunpeng sure.. let me understand the sorting rules correctly.. what is the meaning of sorting first by moduleId and then by dependencies? is your intent to fall back to sorting by dependencies if the moduleId is identical, or are both parts always meant to influence the sort?

[fkleuver]

Actually, let me ask a different question. At what point / where / when do you decide that module A should come before module B?

[3cp]

@fkleuver I am talking about the simple a,b,c sorting snippet above. Forget about the cli sorting, cli sorting is to ensure order of shimmed packages.

For instance, I want to make sure string "a" is after string "c", and don't care about other strings (don't care about the order between "a" and "b", or "b" and "c", or "x" and "y".

[3cp]

@chabou-san you can temporarily work around this issue with wrapShim.

wrapShim delays executing of bootstrap js to the time you do import 'bootstrap';.

{
  "name": "bootstrap",
  "path": "../node_modules/bootstrap/dist",
  "main": "js/bootstrap.min",
  "deps": ["jquery"],
  "wrapShim": true
},

[fkleuver]

For it to be stable, you need to include the in-between values in the sort as well (less-than operator). I believe this should work (you don't necessarily have to have the last a < b ? -1 : 1 ):

function compare(a, b) {
  if (a === b) return 0;
  if (a === 'a' && (b === 'c' || b < 'c')) return 1;
  if (b === 'a' && (a === 'c' || a < 'c')) return -1;
  return a < b ? -1 : 1;
}

[3cp]

Thx @fkleuver I will see what I can do.

[fkleuver]

Basically you just want to be able to tell any given two modules "you should come after this one and you should come after that one", which you might need to tell to different combinations multiple times right? For dependency order. Or am I wrong

[fkleuver]

Might be helpful, here's how rollup approaches this https://github.com/rollup/rollup/blob/841a5366988c945e7cd342b05a8aa8eb9c3411a0/src/utils/execution-order.ts

[3cp]

I have two choices:

1. the simple one: remove wrapShim option, enforce wrapShim for all shim packages. This might not be a bad idea, I need to read more and study on this topic.

2. the complex one: fix the sorting.
   I think I need to dice the packages into

• all packages not involved in shim
• packages group a for a shim tree (e.g. jquery -> bootstrap)
• packages group b for another shim tree

Then sort them separately.

[fkleuver]

I don't know enough about the cli internals to be able to decipher the precise implications of what you just described, unfortunately. Could you summarize into a bullet list of abstract sorting rules? e.g. "if a is a shim and b is ... then a comes before b". I'll be able to help you more easily with the sorting then.

[3cp]

To make a legacy lib into AMD module, we use shim config, so for bootstrap v3, we says "deps": ["jquery"] to identify it depends on jquey. Since bootstrap v3 is plain old JavaScript, it assumes global jQuery object is ready, hence we need to bundle jquery before bootstrap v3.

The sorting rule is, for shimmed package "foo", if it depends on "bar", ensure package "bar" is before "foo".

Pseudo code

function sort(packageA, packageB) {
  let aName = packageA.name;
  let bName = packageB.name;
  let aDeps = packageA.shimDeps;
  let bDeps = packageB.shimDeps;

  // a must be after b
  if (aDeps && aDeps.includes(bName)) return 1;
  // b must be after a
  if (bDeps && bDeps.includes(aName)) return -1;
  
  // don't care
  return 0;
}

[fkleuver]

How about a regular topological sort then?

function sort(packages) {
  const sorted  = [];
  const visited = {};

  packages.forEach(function visit(current) {
    const { name, deps } = current;
    visited[name] = true;
    deps.forEach(dep => {
      if (!visited[dep]) {
        visit(dep);
      }
    });
    if(sorted.indexOf(current) === -1) {
      sorted.push(current);
    }
  });
  return sorted;
}

var pkg1 = { name: 'pkg1', deps: [] };
var pkg2 = { name: 'pkg2', deps: [] };
var pkg3 = { name: 'pkg3', deps: [] };
var pkg4 = { name: 'pkg4', deps: [] };
var pkg5 = { name: 'pkg5', deps: [] };

pkg2.deps.push(pkg4);
pkg3.deps.push(pkg4);

pkg1.deps.push(pkg3);

pkg2.deps.push(pkg1);

pkg5.deps.push(pkg1);
pkg5.deps.push(pkg2);
pkg5.deps.push(pkg3);
pkg5.deps.push(pkg4);

var packages = [pkg1, pkg2, pkg3, pkg4, pkg5]

packages;
// 0: {name: "pkg1", deps: Array(1)}
// 1: {name: "pkg2", deps: Array(2)}
// 2: {name: "pkg3", deps: Array(1)}
// 3: {name: "pkg4", deps: Array(0)}
// 4: {name: "pkg5", deps: Array(4)}

sort(packages);

packages;
// 3: {name: "pkg4", deps: Array(0)}
// 2: {name: "pkg3", deps: Array(1)}
// 0: {name: "pkg1", deps: Array(1)}
// 1: {name: "pkg2", deps: Array(2)}
// 4: {name: "pkg5", deps: Array(4)}

[3cp]

Awesome!

[fkleuver]

let me know if works / doesnt work :)

[3cp]

It works, slightly changed to

function sort(packages) {
  const sorted  = [];
  const visited = {};

  packages.forEach(function visit(current) {
    const { name, deps } = current;
    if (visited[name]) return; //here
    visited[name] = true;
    deps.forEach(visit); //here
    sorted.push(current); // and here
  });
  return sorted;
}

[3cp]

Hi @chabou-san can you try "aurelia-cli": "huochunpeng/cli#fix-shim-order-again"?

[chabou-san]

Ok, I can confirm it's working for me now. 👍 Many thanks. 😄

@chabou-san you can temporarily work around this issue with wrapShim.

Don't worry, I've always had a workaround by setting jQuery in the "prepend" array property in aurelia.json, or even in the usual "dependencies" too. But it's not needed anymore, and jQuery is now correctly sorted AND auto-traced. 😄