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

Improved layering between app and tests bundles #620

Merged
merged 5 commits into from
May 27, 2024
Merged

Conversation

mansona
Copy link
Member

@mansona mansona commented Apr 18, 2024

Fixes #503
Closes #512

@mansona mansona force-pushed the improved-layering branch from a727b4e to a934952 Compare April 24, 2024 15:48
@ef4 ef4 force-pushed the improved-layering branch from a934952 to a3ded95 Compare May 27, 2024 13:16
@ef4 ef4 force-pushed the improved-layering branch from a3ded95 to c670b4d Compare May 27, 2024 13:53
@ef4 ef4 marked this pull request as ready for review May 27, 2024 14:02
@ef4
Copy link
Collaborator

ef4 commented May 27, 2024

@simonihmig and I pushed this forward and have passing tests. I published it as 2.7.3-alpha.0. @simonihmig is going to test it under real-app conditions before we merge.

We think this is a minimum set of changes to fix the bug, but it leaves undone a lot of cleanup refactoring that could eliminate unused functionality in ember-auto-import and simplify the codebase. Essentially now we are trusting webpack to manage which deps get into app vs tests, so all the places where embroider ember-auto-import is trying to manage that ahead of webpack are unnecessary now.

@ef4 ef4 added the bug Something isn't working label May 27, 2024
@simonihmig
Copy link
Contributor

I tested this on our monorepo with many hundreds of (test-)apps and >150 instances of workarounds for failing tests due to this bug. After reverting those workarounds and running CI with the alpha release, >99% of apps were passing!

The remaining failures were related to the more rare case when an app does not have any eai-handled imports, only the tests have. In that case there wouldn't be any app script, and with the previous implementation no chunks would be inserted into the index.html. This was fixed with 730270c, which I just pushed into this branch.

🎉 With that I have a perfectly green build now! 🎉

@ef4 ef4 force-pushed the improved-layering branch from 1fde554 to aa30bff Compare May 27, 2024 22:22
@ef4 ef4 merged commit e47c6e7 into main May 27, 2024
124 checks passed
@ef4 ef4 deleted the improved-layering branch May 27, 2024 22:40
@github-actions github-actions bot mentioned this pull request May 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Let webpack handle test vs app layering
3 participants