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

Let webpack handle test vs app layering #503

Open
ef4 opened this issue Feb 12, 2022 · 5 comments · May be fixed by #512 or #620
Open

Let webpack handle test vs app layering #503

ef4 opened this issue Feb 12, 2022 · 5 comments · May be fixed by #512 or #620

Comments

@ef4
Copy link
Collaborator

ef4 commented Feb 12, 2022

At present, we generate two separate entrypoints to hand to webpack (one for app, one for tests). We follow the same pattern that normal ember apps do, such that the tests are allowed to assume that the app is necessarily always loaded first, so a dependency that is used by the tests should be left out of the test entrypoint if it's also used by the app.

However, it would be better to switch to layering the tests and app the way embroider does, by creating an explicit dependency from the test entrypoint to the app entrypoint. This lets webpack handle the deduplication, and it can take into account more resolution detail than we can.

A case that fails today is having app code that imports from "some-dependency/thing.js" and test code that imports from "some-dependency/thing". Both resolve to the same module, but it will actually load and execute twice because ember-auto-import fails to deduplicate it.

@ef4
Copy link
Collaborator Author

ef4 commented Mar 10, 2022

@SergeAstapov pointed out that his causes even more significant trouble because having separate entrypoints for app and tests on the same page causes webpack to have duplicated runtime caching, so even when modules aren't duplicated they can get evaluated multiple times and not share state.

@knownasilya
Copy link
Contributor

@ef4
Copy link
Collaborator Author

ef4 commented Aug 29, 2022

No, I think that is probably #504

@runspired
Copy link

runspired commented Oct 16, 2022

even if we solve why ember-auto-import doesn't work when treeForAddon is custom (all babel plugins present, but I think the other custom tree stuff auto-import does is the issue, see #543) this issue will prevent EmberData from converting anything to V2 Addons as we rely on module state for quite a large number of things.

you can see this at play in the branch mentioned there. If you turn-off rollup for the build the tests will load and run but the tests that actually require the use of the v2-addon will fail. If however you leave rollup on and import @ember-data/tracking/-private in the test file such that it will be present to the runtime but you only get one copy, then the same tests will pass.

@DingoEatingFuzz
Copy link

I ran into this last night with an instanceof check not working due to the same class being executed twice (so functionally equivalent but referentially different).

Happy to see there's already a PR open ❤️

thoov added a commit to thoov/ember-auto-import that referenced this issue Feb 10, 2023
Currently, ember-auto-import has two conceptual buckets where it places the imports it has found (an app bucket and a tests bucket). While this works, it has two potential problems. The first is with imports from within lazy engines as they get placed inside of the "app" bucket. This means that instead of these asset loading lazily they are instead eagerly loaded as part of the main app bundle. The second problem is that modules can be "duplicated" in each bucket and can cause problems with module states' being different depending on the importer.

In order to fix this, this PR creates an individual entry file (or bucket) for every addon that is doing an import. It then creates import links from each entry file to its rightful parent (ie a nested addon from a lazy engine would only be imported from the lazy engine's entry file and then the lazy engine's entry file is import from the app's entry file). Additionally, the tests entry file now imports the app entry file which creates a conceptual graph for webpack to properly traverse such that modules will not be duplicated.

Fixes: embroider-build#503
thoov added a commit to thoov/ember-auto-import that referenced this issue Feb 10, 2023
Currently, ember-auto-import has two conceptual buckets where it places the imports it has found (an app bucket and a tests bucket). While this works, it has two potential problems. The first is with imports from within lazy engines as they get placed inside of the "app" bucket. This means that instead of these asset loading lazily they are instead eagerly loaded as part of the main app bundle. The second problem is that modules can be "duplicated" in each bucket and can cause problems with module states' being different depending on the importer.

In order to fix this, this PR creates an individual entry file (or bucket) for every addon that is doing an import. It then creates import links from each entry file to its rightful parent (ie a nested addon from a lazy engine would only be imported from the lazy engine's entry file and then the lazy engine's entry file is import from the app's entry file). Additionally, the tests entry file now imports the app entry file which creates a conceptual graph for webpack to properly traverse such that modules will not be duplicated.

Fixes: embroider-build#503
thoov added a commit to thoov/ember-auto-import that referenced this issue Feb 10, 2023
Currently, ember-auto-import has two conceptual buckets where it places the imports it has found (an app bucket and a tests bucket). While this works, it has two potential problems. The first is with imports from within lazy engines as they get placed inside of the "app" bucket. This means that instead of these asset loading lazily they are instead eagerly loaded as part of the main app bundle. The second problem is that modules can be "duplicated" in each bucket and can cause problems with module states' being different depending on the importer.

In order to fix this, this PR creates an individual entry file (or bucket) for every addon that is doing an import. It then creates import links from each entry file to its rightful parent (ie a nested addon from a lazy engine would only be imported from the lazy engine's entry file and then the lazy engine's entry file is import from the app's entry file). Additionally, the tests entry file now imports the app entry file which creates a conceptual graph for webpack to properly traverse such that modules will not be duplicated.

Fixes: embroider-build#503
thoov added a commit to thoov/ember-auto-import that referenced this issue Feb 10, 2023
Currently, ember-auto-import has two conceptual buckets where it places the imports it has found (an app bucket and a tests bucket). While this works, it has two potential problems. The first is with imports from within lazy engines as they get placed inside of the "app" bucket. This means that instead of these asset loading lazily they are instead eagerly loaded as part of the main app bundle. The second problem is that modules can be "duplicated" in each bucket and can cause problems with module states' being different depending on the importer.

In order to fix this, this PR creates an individual entry file (or bucket) for every addon that is doing an import. It then creates import links from each entry file to its rightful parent (ie a nested addon from a lazy engine would only be imported from the lazy engine's entry file and then the lazy engine's entry file is import from the app's entry file). Additionally, the tests entry file now imports the app entry file which creates a conceptual graph for webpack to properly traverse such that modules will not be duplicated.

Fixes: embroider-build#503
@mansona mansona linked a pull request Apr 24, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants