-
Notifications
You must be signed in to change notification settings - Fork 2.5k
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Fix preload application package import #12964
Conversation
@vince-fugnitto I imagine we need a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for this fix! I can confirm that preload and production build works again for browser with this fix.
I'm sorry for having introduced this issue with #12897, I didn't expect to break the preload / production build with this package level import.
To reveal such an issue in the future early, I'd suggest to run a very simple playwright test on the production mode on every change, which just checks if the application loads correctly. It'd be very easy to set up and I'd be happy to open a PR for that. WDYT?
All good, I know how it feels to have broken something accidentally ;)
I wanted to propose something similar. It's not like I had tests for the preload stuff, but I was still suprised that our tests didn't catch the issue. I'm all in for that 👍 |
Thank you! :-)
Great, I'll open a PR in the next 1-2 days! |
For future references: I've opened #12965 to add a smoke test for the production build |
Sounds good, we can mention it during the dev meeting (tomorrow) and perform the release. |
What it does
Closes #12962
Fixes the import to use one that doesn't rely on node.js dependencies.
How to test
Confirm that the preload functionality behaves as expected.
Review checklist
Reminder for reviewers