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

Fix compatibility with Ember 3.27+ when using compileModules: false. #402

Merged
merged 1 commit into from
May 6, 2021

Conversation

rwjblue
Copy link
Member

@rwjblue rwjblue commented May 6, 2021

When using libraries that leverage compileModules: false (commonly used to transpile vendor tree's without introducing a wrapping AMD module) on Ember 3.27+ (where we don't transpile away the various @ember/* modules any longer) we would leave the debug module imports alone during transpilation. This resulted (in most cases) in there being a bare unwrapped and untranspiled import statement in vendor.js. This would then fail everything.

This changes the logic slightly to continue transpiling to globals mode when compileModules: false is set.

Note: this is really just a stop gap solution (and will still cause deprecations to be emitted, addons that are currently leveraging compileModules: false will need to make some changes for Ember 4.0 (where a global window.Ember will not be ambiently available).

When using libraries that leverage `compileModules: false` (commonly
used to transpile `vendor` tree's without introducing a wrapping AMD
module) on Ember 3.27+ (where we don't transpile away the various
`@ember/*` modules any longer) we would leave the debug module imports
alone during transpilation. This resulted (in most cases) in there being
a bare unwrapped and untranspiled `import` statement in `vendor.js`.
This would then fail **everything**.

This changes the logic slightly to continue transpiling to globals mode
when `compileModules: false` is set.

Note: this is really just a stop gap solution (and will still cause
deprecations to be emitted, addons that are currently leveraging
`compileModules: false` _will_ need to make some changes for Ember 4.0
(where a global `window.Ember` will not be ambiently available).
@rwjblue rwjblue added the bug label May 6, 2021
@rwjblue rwjblue merged commit 127ade1 into master May 6, 2021
@rwjblue rwjblue deleted the fix-compileModules-false-on-ember-3.27 branch May 6, 2021 00:41
@scalvert
Copy link

scalvert commented May 6, 2021

Thanks for this, @rwjblue. The comment/test looks good.

@ef4
Copy link
Collaborator

ef4 commented May 10, 2021

I don't think this is complete. It will transpile away import { assert } from '@ember/debug' but it will still leave things like import Ember from 'ember'. The whole modules API polyfill would need the same special handling here, not just the debug macros. But also, embroider stage1 forces compileModules: false. If that starts re-enabling the modules API polyfill even on new ember versions it will cause needless deprecation spew in embroider builds.

People authoring their code as ES modules and then passing it to ember-cli-babel with compileModules: false should not be surprised when they get untranspiled modules! That is exactly the name of the option!

IMO, this PR should be reverted and we should tell people it's their own bug if they're using import and compileModules: false together. It worked by accident on earlier ember versions because ember was "cheating" on modules. Their code was never properly using ES modules anyway, because if they had tried to import anything other than the fake modules it would have blown up.

I can't emphasize strongly enough what a dead-end it is to try to use ES modules in treeForVendor. If you want ES modules, get out of treeForVendor. While you're still in treeForVendor, use Ember global instead of imports.

@ef4
Copy link
Collaborator

ef4 commented May 11, 2021

I have confirmed that this change breaks a wide swath of Embroider's test suite. I think it breaks every app on ember-source 3.27 using embroider.

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

Successfully merging this pull request may close these issues.

3 participants