Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bug introduced by #574 when using conditional importSync #582

Closed
simonihmig opened this issue May 26, 2023 · 1 comment · Fixed by embroider-build/embroider#1468
Closed

Comments

@simonihmig
Copy link
Contributor

Reproduction: https://github.com/simonihmig/eai-cached-decorator-polyfill-reproduction

What's happening:

  • An app with latest eai uses the v2 addon ember-headless-table
  • ember-headless-table supports older Ember versions, so uses ember-cached-decorator-polyfill for @cached support
  • but it declares only an optional peer dependency on it. So apps on newer Ember version can omit this.
  • it does so by applying the polyfill's babel plugin to its own code as part of its own (publish-time) build
  • the static v2 code in /dist looks like this:
    let cached = macroCondition(dependencySatisfies('ember-source', '>= 4.1.0-alpha.0')) ? importSync('@glimmer/tracking').cached : importSync('ember-cached-decorator-polyfill').cached;
    
  • when the app builds, this code is transformed by @embroider/macros to:
    let cached=(0,_embroider_macros_src_addon_runtime__WEBPACK_IMPORTED_MODULE_1__.macroCondition)(true)?(0,_embroider_macros_src_addon_es_compat__WEBPACK_IMPORTED_MODULE_0__[\"default\"])(__webpack_require__(/*! @glimmer/tracking */ \"@glimmer/tracking\")).cached:(0,_embroider_macros_src_addon_es_compat__WEBPACK_IMPORTED_MODULE_0__[\"default\"])(__webpack_require__(/*! ember-cached-decorator-polyfill */ \"ember-cached-decorator-polyfill\")).cached;
    
    This is a non-production build, so still contains some runtime stuff. But you see the macroCondition() condition compiled to true, so it will import from @glimmer/tracking rather than ember-cached-decorator-polyfill
  • so far everything seems fine...
  • But, the AMD shim that eai generates looks (shortened) like this:
    d('ember-headless-table', [..., 'ember-cached-decorator-polyfill', ...], function() { ... })
    
  • in the browser, this then fails with Error: Could not find module ember-cached-decorator-polyfill imported from ember-headless-table

This was revealed in a renovate PR updating our app to eai to 2.6.3. When we still had ember-cached-decorator-polyfill, this didn't show up (but another problem - @ef4 this was the state of the PR when you looked into it back then). So now after removing ember-cached-decorator-polyfill, eai 2.6.3 (and not older versions!) causes that error.

I think it must have to do with #574, which changed the way how dependencies from v2 addons (here: ember-headless-table) to v1 addons (here: ember-cached-decorator-polyfill) are handled.

I wonder how we can deal with that issue? Do we have to make the dependencies checker here so smart that it will understand what @embroider/macros do, and that our importSync('ember-cached-decorator-polyfill') is actually dead code and needs to be ignored? 🤔

/cc @NullVoxPopuli

@ef4
Copy link
Collaborator

ef4 commented Jun 6, 2023

Discussed at embroider weekly meeting, we plan to fix this particular case by making macroCondition sensitive to whether its predicate actually needs a runtime implementation or not. dependencySatisfies does not, so we could do the branch elimination even in dev.

(This is not a generation solution to importSync forcing optional deps to be non-optional, but it covers a very common case)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants