v2 addons don't always get ideal execution order

[simonihmig]

I pulled my hair with some very broken behavior in an app. Luckily I was able to create a minimal reproduction. So this is a super weird issue, be prepared...

Context

I believe this has to do with the following circumstances:

• a v2 addon (in this case tracked-redux) imports a v1 addon (ember-tracked-storage-polyfill)
• the app imports the v2 addon (tracked-redux, directly or through some other addon), and it also(!) imports the v1 addon (ember-tracked-storage-polyfill, again directly or through some other addon)
• during module resolution, the import of the v2 addons happens before the other v1 addon is imported through the app or some other addon. So the v1 addon (ember-tracked-storage-polyfill) module is loaded (by loader.js) the first time through the v2 addon.

Note that this reproduction uses tracked-redux, as this is also the v2 addon used in the real app, and I didn't want to have to create another (simpler) v2 addon just for the reproduction. But I strongly believe the error here is not related to redux or something else in that v2 addon, as it doesn't do anything unusual, with regards to imports or anything related. AFAICT.

Reproduction

• In the application route, tracked-redux is imported
• tracked-redux imports getValue from ember-tracked-storage-polyfill and assigns it to an alias consumeTag here
• However at the time the alias is assigned, the imported module of ember-tracked-storage-polyfill (webpack-require) is an empty object, so consumeTag is undefined. See this screenshot while debugging:
  
• when debugging into that webpack-require call, and following the wrapper module of eai that calls again into Ember's AMD-like require by loader.js, you can see that the module is in pending state, and indeed the returned mod.module.exports is just an empty object:
  
• so when the consumeTag alias is first used, this fails of course with "consumeTag is not a function", as you can also see in CI
• however putting a break point here, shows that at this (later!) time the imports of ember-tracked-storage-polyfill have actually been resolved. The code still fails of course due to the fact that an alias has been assigned eagerly, which is still undefined. See:
  
• so this seems to be somewhat timing related. While not assigning an alias would maybe have helped, it still shows that the semantics of ESM imports being synchronous have not been fulfilled here, right?

More to come

I was actually able to workaround that problem, however in weird and unexpected ways...

1. If I remove the import of the v1 addon (ember-tracked-storage-polyfill) from the app, so that only the v2 addon imports it, this resolves the issue. This scenario is replicated in "Fix" broken import by removing other import simonihmig/broken-import-reproduction#2
2. If I change the order of imports, so that the v2 addon imports the v1 addon after it has been already imported elsewhere (here the app), then this also works! 😱 See "Fix" broken import by changing import order simonihmig/broken-import-reproduction#1

Both changes illustrated in those PRs make the test pass! 🤔

Tbh I am not really sure where the bug is, if it's eai, or maybe even loader.js. Though I debugged quite a bit through those module resolution code of loader.js, I didn't really follow its inner logic. But as it seems this is related in some way to v2 addons entering the playing field here, I thought I file it here first!

[ef4]

Thanks for the great bug report. I see what's happening here. It involves the interaction of three things:

1. The ES module spec allows this kind of behavior if there's a module cycle. That is, if imports form a cycle, some module will see a copy of the another module that isn't initialized yet:

   import { consumeTag } from 'ember-tracked-storage-polyfill';
   
   // if our module and ember-tracked-storage-polyfill participate in a cycle,
   // then `consumeTag` could be undefined HERE
   
   export function whatever() {
     // But by the time somebody else calls this function and we get here, it's no longer undefined. 
     // Because imported bindings are not like regular assignments -- they are live bound, and 
     // if the value changes *inside* the module it also changes here!
   }

2. tracked-redux is making itself unnecessarily vulnerable to this kind of problem by copying the binding in module scope:

   import { getValue } from 'ember-tracked-storage-polyfill';
   // here, getValue is undefined
   const consumeTag = getValue;
   const consumeCollection = (node) => {
       // here getValue is defined but consumeTag is still undefined!
       consumeTag(tag); 
   };

   The fix is to avoid making your own independent variable:

   import { getValue as consumeTag } from 'ember-tracked-storage-polyfill';
   
   const consumeCollection = (node) => {
       consumeTag(tag); 
   };

   This code is robust even against a cycle.

3. The final piece of the puzzle is, why is there a cycle? That's happening because loader.js doesn't really follow the module spec. When it sees this:

   import { createStore } from 'tracked-redux';
   import { createStorage } from 'ember-tracked-storage-polyfill';

   If neither module has already been run, it effectively locks in their relative order and says that tracked-redux must run before ember-tracked-storage-polyfill because it appears first. That's incorrect behavior, and it introduces a new phantom dependency that acts the same as a cycle.

Should we fix loader.js? Maybe, but I bet people are accidentally relying on this wrong execution order.

This bug would not occur under embroider because there ember-tracked-storage-polyfill will get rewritten into a v2 addon and the dependencies between tracked-redux and ember-tracked-storage-polyfill are all handled by webpack, not loader.js.

So I think your best bet is simonihmig/tracked-redux#123.

[simonihmig]

While not assigning an alias would maybe have helped, it still shows that the semantics of ESM imports being synchronous have not been fulfilled here, right?

So this was apparently the wrong expectation I had...

The ES module spec allows this kind of behavior if there's a module cycle

Got it. As said above, I thought I can expect the imports to be resolved synchronously and under any circumstance. But in hindsight I see why that cannot work in the case of cycles. Should maybe start to study the spec... 😏

Thanks so much for taking the time to go through this in detail!

[ef4]

I went to reproduce this loader.js behavior in isolation and realized it is a problem with the way ember-auto-import is interacting with loader.js. When ember-auto-import isn't involved, loader.js does get the order right here.

ember-auto-import manages the boundary between loader.js and webpack's loader, and up until recently that has always been one-way. You would auto-import non-ember packages and they wouldn't in turn depend on any ember packages owned by loader.js. But now that v2 addons are handled by ember-auto-import, the relationship goes both ways.

We register a stub with loader.js that gets the package from webpack, but our stub doesn't expose any dependency list to loader.js, which means loader.js can't figure out the ideal place that our stub should fit in the execution order. So it may run before some of its dependencies, as if there was a cycle.

[ef4]

This is probably fixable but it's not easy.

We could run the full Analyzer over v2 addons to know in advance what they're importing, but that's quite expensive and we don't really even have a place where we could easily do it from. Normally they don't get touched until webpack is traversing them, and the only reason webpack is traversing them is because we already wrote the stub modules in webpack's input.

We could discover the dependencies by side-effect from the externalsHandler, but then we have to do something tricky to get that information into the build, since at this point all our inputs to webpack have been provided. Maybe a custom webpack plugin can add the extra information at the end, and then our runtime code would read that dependencies info at the point where it's calling window.define.

[NullVoxPopuli]

I just ran in to this issue with @tracked (from a v2 addon) from @glimmer/tracking being undefined in a consuming app (dummy addon's tests).

Is the path forward to make sure all dependencies of v2 addons are also v2 addons? in that we need @glimmer/tracking, @glimmer/component, and ember-source to all be v2 addons?

[NullVoxPopuli]

well there has to be something fishy with my setup, because all my other v2-addon consuming projects test non-embroider builds, and they worked with @tracked 🤔

[ef4]

That is consistent with this issue though. It's evaluation-order dependent. Lots of projects won't hit it, purely out of luck.

[simonihmig]

@ef4 do we have to reopen this (I don't have perms), given that the fix closing this was reverted in #568, right?