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

Chunk generated for v2 addon is not respecting public entrypoints #543

Closed
runspired opened this issue Oct 15, 2022 · 11 comments
Closed

Chunk generated for v2 addon is not respecting public entrypoints #543

runspired opened this issue Oct 15, 2022 · 11 comments

Comments

@runspired
Copy link

I have a v2-addon that produces two files: -private.js and index.js.

In a v1-addon that has declared a dependency on this v2-addon I import -private.js. This blows up. Inspecting the webpack chunk shows that while both files are included in the chunk only index.js is added into ember's require cache.

@runspired
Copy link
Author

I suspect the issue here may be that the test app (which is where the v2-addon code gets included by ember-auto-import) imports only index.js while the addons import -private.js. It seems ember-auto-import is ignoring imports from addons in favor of the test app.

@runspired
Copy link
Author

As I dig in, I think this might be due to how EmberData addons have a rollup step in their build. At first I thought maybe the babel plugin wasn't getting registered but I added ember-auto-import/babel-plugin to ensure it was there while we were rolling up and still no-dice.

I notice that ember-auto-import typically uses the preprocessor registry, I'm not sure if that approach would work for EmberData because we do not super treeForAddon to avoid consumers paying the price of a double compile. Is there something we need to do to get the preprocessor into the babel/rollup config separate from just adding ember-auto-import/babel-plugin?

@runspired
Copy link
Author

so it appears ember-auto-import/js/analyzer-plugin is present in babel options when we pass them to rollup, so the preprocessor logic appears to be working just fine. That said, I think I'm just going to convert back to a v1-addon.

It surprised me greatly to learn that ember-auto-import was how v2 addons work. This seems like a huge footgun, both to consumers and also to the maintenance and incremental adoption roadmap for getting the community to use embroider. I would have expected the embroider addon-shim to have made a v2-addon compatible in the build as a v1-addon, not to try to load it via this other mechanism.

This scares me greatly, as not only does it mean ember-data is unlikely to be able to incrementally convert to v2-addons very easily, but also the issue in #503 would mean that consumers get multiple copies which is a massive issue for a library like EmberData. In the case of this particular addon where module state was essential it's a non-starter, even were we to figure out why it is not getting properly consumed by the v1-addons we would have ended up with two copies and thus two different module states.

@runspired
Copy link
Author

If anyone wants to explore fixing this in the future, I left a branch with the v2 version here: ember-auto-import-v2-addon-bug in emberjs/data

@ef4
Copy link
Collaborator

ef4 commented Oct 16, 2022

I would have expected the embroider addon-shim to have made a v2-addon compatible in the build as a v1-addon

That is exactly what it does though. It's just that "v1-addon" is a very flexible beast.

It surprised me greatly to learn that ember-auto-import was how v2 addons work.

Can you expand what you mean by "ember-auto-import" in that sentence? Like, are you surprised they get built by webpack? Are you surprised that their consumers' import statements get analyzed?

Because both of those things are pretty strongly implied by the semantics of v2 addons. If a v2 addon's module doesn't get imported by somebody, it doesn't go into the build. They must participate in a global module graph. Giving classic ember app and v1 addon code access to a pull-based global module graph is what ember-auto-import does.

I'm all in favor of fixing a bug like #503 and would be happy to have help with it. It's just a bug, we do know how to solve it because the solution is to make ember-auto-import work slightly closer to how embroider already does in this area.

@runspired
Copy link
Author

I would have expected the output of a v2-addon + shim to have participated in the build as-if it were a v1-addon by adding the appropriate addon tree, and not via some separate mechanism.

In fact looking at the shim, this is exactly what the shim does for every asset type except the addon source code. It would be trivial to plug it in as a conventional addon tree, so that is what I'm going to do for now.

@ef4
Copy link
Collaborator

ef4 commented Oct 16, 2022

I would have expected the output of a v2-addon + shim to have participated in the build as-if it were a v1-addon by adding the appropriate addon tree, and not via some separate mechanism.

treeForAddon means "unconditionally append all these modules to the build and register them with the runtime AMD loader under whatever names they want". Is that what you're really advocating?

@runspired
Copy link
Author

Yes. It seems a far better middle ground than unexpectedly ejecting entirely from ember-cli, especially considering ember-auto-import is apparently incompatible with any addon that customizes it's addon tree...

@ef4
Copy link
Collaborator

ef4 commented Oct 16, 2022

It's not middle ground though. It's just the old ground. It's a v1 addon and would get us no closer to having a fully static build.

@runspired
Copy link
Author

It's absolutely not the old ground to have a build that produces precompiled assets but supplies them to a consuming app via the old hooks. You're still doing less at build, you're still now a conventional lib structure with conventional tooling compatibility. The only difference is you end up with all the built files in the legacy module system instead of the thread-pull, but this maximizes compatibility for the old system while still allowing a new build system like embroider to do something else by treating it as a traditional lib.

@ef4
Copy link
Collaborator

ef4 commented Oct 17, 2022

Now that I've had a chance to debug your branch, the problem isn't really related to v2 addons per se. It would fail the same if you tried to access any package via ember-auto-import.

The problem is that the ember-data packages depend on ember-auto-import but don't actually configure it properly. I submitted emberjs/data#8222 to fix that.

If you're going to disable a standard feature like the preprocessor registry, you're going to be forced to deal with this kind of stuff manually.

@ef4 ef4 closed this as completed Oct 17, 2022
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

No branches or pull requests

2 participants