-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Emit our packages in treeforAddon and treeForAddonTestSupport #20349
base: main
Are you sure you want to change the base?
Conversation
One actual semantic change I made here is that there was some probably-unintentional duplication between the default and test bundles. In an app this would manifest as having code duplicated between vendor.js and test-support.js, with identical modules overwriting each other in the runtime loader. I removed I also removed |
Another change here is that the bundled ember was wrapping loader.js in an extra layer of loader API that seems unnecessary. |
@wycats @gitKrystan this is relevant to our discussion last week. |
I've been trying to dogfood this in some or our apps. In our ~1yo app (running embroider), I am running into: Build Error (WaitForTrees)
Cannot find module 'ember-source/vendor/ember/ember-template-compiler' from '/private/var/folders/xx/2vtrs0m95q36gnbsvy_2293c0000gn/T/embroider/24c694'
Stack Trace and Error Report: /var/folders/xx/2vtrs0m95q36gnbsvy_2293c0000gn/T/error.dump.ac984c420315c3f4d362f090ce7682dd.log It turns out that this line in embroider/compat-app looks for But this PR puts it in If I change the ember-cli-build
then I get this error during build: Cannot find module '/Users/krystanhuffmenne/Code/tilde/okapi/okapi-gui/node_modules/ember-source/dist/vendor/ember-template-compiler.js'
Require stack:
- /Users/krystanhuffmenne/Code/tilde/okapi/okapi-gui/node_modules/ember-cli-htmlbars/lib/utils.js
- /Users/krystanhuffmenne/Code/tilde/okapi/okapi-gui/node_modules/ember-cli-htmlbars/lib/ember-addon-main.js
- /Users/krystanhuffmenne/Code/tilde/okapi/okapi-gui/node_modules/ember-cli/lib/models/package-info-cache/package-info.js
- /Users/krystanhuffmenne/Code/tilde/okapi/okapi-gui/node_modules/ember-cli/lib/models/package-info-cache/index.js
- /Users/krystanhuffmenne/Code/tilde/okapi/okapi-gui/node_modules/ember-cli/lib/models/project.js
- /Users/krystanhuffmenne/Code/tilde/okapi/okapi-gui/node_modules/ember-cli/lib/utilities/get-config.js
- /Users/krystanhuffmenne/Code/tilde/okapi/okapi-gui/node_modules/ember-cli/lib/utilities/instrumentation.js
- /Users/krystanhuffmenne/Code/tilde/okapi/okapi-gui/node_modules/ember-cli/lib/cli/index.js
- /Users/krystanhuffmenne/Code/tilde/okapi/okapi-gui/node_modules/ember-cli/bin/ember But if I change the above-mentioned line in embroider/compat-app to In fact, I am having the same issue with a test app created with |
Everything appears to work on a fresh app created with But in our non-embroider app running ember-cli 4.11, I get no build errors, but a runtime error: vendor.js:7612 Uncaught ReferenceError: Ember is not defined
at vendor.js:7612:119
at vendor.js:7681:3
vendor.js:266 Uncaught Error: Could not find module `@ember/application` imported from `direwolf/app`
at missingModule (vendor.js:266:11)
at findModule (vendor.js:277:7)
at Module.findDeps (vendor.js:187:24)
at findModule (vendor.js:281:11)
at requireModule (vendor.js:43:15)
at direwolf.js:55708:13 I'm having more trouble debugging this one and would love any suggestions. |
@ef4 @gitKrystan I wonder if we should take the opportunity to express these locations in an (It might have other problematic implications, though, since some tools treat the presence of an |
During dog-fooding non-embroider apps with @wycats, we're noticing that multiple addons (e.g. ember-cli-deprecation-workflow) expect To reproduce this issue, add Additionally, we attempted to backport this change to Ember 3.28 and found that somehow it loses the Ember global. We are continuing to investigate these issues as well as doing more dogfooding in Embroider apps, but we'd welcome any feedback if you have ideas. |
This is not expected, nothing in the PR should have intentionally affected that. Maybe the double-loader shenanigans I removed was facilitating that behavior. But in general, in a classic build, everything emitted by treeForAddon will definitely still be |
I investigated this further.
That isn't quite the whole story. This PR doesn't change what ember-cli-deprecation-workflow is injecting a script, not a module. Accessing ember's modules from a script is an ill-defined thing to do. It's timing-dependent. In the current implementation, it happens to work because of the relative timing of where ember's stuff and ember-cli-deprecation-workflow's stuff end up in vendor.js. Nothing is actually guaranteeing that timing, and the order happens to change with this PR. |
This class of problem is why I have been saying that deprecating the AMD loader is one of our high priorities. |
Is this still relevant? It is a draft PR |
This is a step toward making ember-source a less weird and badly-behaved addon. By emitting its code in treeForAddon instead of treeForVendor, ember-source actually exposes its ES modules to the rest of the system and can benefit from ember-auto-import or embroider.
So far this draft PR is enough to make an example application boot and pass tests. This repo's tests won't be passing yet because they're hard-coded to the old way the assets were generated.
Next I'd like to switch the prepublication build to use just rollup, without broccoli & ember-cli, because it would be easier to produce a conventional addon structure and easier to reimplement the test suite as modules rather than scripts.