Interoperability issue of embroider and loader.js

[simonihmig]

seems I have a talent for the ugly issues (after #509), just spent most of the day pulling my hair out on this one...

Tests were failing for me randomly in an addon, but only when building with Embroider. It turned out that a component could be wrongly resolved, depending on the timing.

Having components A and B, and B extending from A, if B was resolved before A, everything seems fine. The other way around however, when resolving B you would get back the module for A.

After a long debugging session, I think I have found the reason. ember-resolver would try to get the default export of a module if available here.

This works fine in a classic build, when you look at the compiled vendor.js, then each module in its AMD form has an explicit default export, like this:

define(..., function(...) {
  let MyComponent = ...
  _exports.default = MyComponent;
});

This is however not the case with a Webpack build.

Now the problem lies with loader.js, which mutates a module export by creating a default export here, if it is not already present (remember, it is present in a classic build) and the makeDefaultExport option is set (which it is by default).

But in the case of a webpack-build module that exports a component, this creates a default property on the exported class! 😱

So when B is resolved first, it creates B.default (pointing to B again). If then A is resolved, A.default does not exist, so also A.default is created (pointing to a). This side-effect of extending the classes seems ugly enough, but at least it works so far.

Now the other way around: A is resolved first, A.default is created, referencing A again. Now it is the turn for B, and here this mess breaks everything: so as B extends from A, B.default is already defined, but it still points to A! So no default is created for B by loader.js, as it already inherited it from A. But ember-resolver will now use B.default (same place here) which still refers to A. So by trying to resolve component B, you get a reference to class A! 🙈

When patching my local loader.js, specifically setting makeDefaultExport to false here, all the failing tests were gone. So that confirms my observation.

Tbh, the choice to also extend a function as the module export here seems odd to me! 🤔

[ef4]

Thanks @simonihmig, nice work.

[simonihmig]

Here is a PoC PR: #540

[rwjblue]

Now the problem lies with loader.js, which mutates a module export by creating a default export here, if it is not already present (remember, it is present in a classic build) and the makeDefaultExport option is set (which it is by default).

FWIW, I want to remove that functionality. See ember-cli/loader.js#114 (which was what lead to creating the option you mentioned ember-cli/loader.js#129). I'd like to try to push forward the work in ember-cli/loader.js#128 and deprecate/remove that functionality in loader.js completely...

[rwjblue]

I'd prefer if we could avoid having differing behaviors here between "classic build" and Embroider builds, so making loader.makeDefaultExports = false a default option seems better to me...

[simonihmig]

I'd like to try to push forward the work in ember-cli/loader.js#128 and deprecate/remove that functionality in loader.js completely...

@rwjblue Thanks for chiming in, and adding this context! Do you expect this to be done (i.e. remove that functionality, release a new major) in a relatively short period of time (like days instead of weeks)? Or should we try to get a workaround added to embroider as an interim solution?

The thing is that loader.js is a direct app dependency, so unless the user actively updates it (with the changes you proposed, once released), this bug will still be present. So we probably should add a workaround here anyway, don't we? Maybe something like #540, just adding a version check for loader.js to add that only for versions that don't include the changes you proposed?

I'd prefer if we could avoid having differing behaviors here between "classic build" and Embroider builds, so making loader.makeDefaultExports = false a default option seems better to me...

I agree!

[rwjblue]

Do you expect this to be done (i.e. remove that functionality, release a new major) in a relatively short period of time (like days instead of weeks)?

We definitely can get it done quickly, but I'd need help reviving that PR and getting it over the line. I've got time to help with reviews/releases/etc but not impl.

Or should we try to get a workaround added to embroider as an interim solution?

The thing is that loader.js is a direct app dependency, so unless the user actively updates it (with the changes you proposed, once released), this bug will still be present. So we probably should add a workaround here anyway, don't we? Maybe something like #540, just adding a version check for loader.js to add that only for versions that don't include the changes you proposed?

IMHO, landing the fix in your other PR is totally fine, but I want to also land the loader.js changes so that we attack the issue from both angles and eventually as projects upgrade the divergence is reduced.

[simonihmig]

but I'd need help reviving that PR and getting it over the line

Sure. But it seems the PR's author has addressed most of your feedback after your initial review over there! Could you re-review this, or what would the next step be? If the PR needs someone to continue the work, in case the original submitter is not available anymore - it's been >3 years 🙀 - then please ping me, maybe I can help there!