-
Notifications
You must be signed in to change notification settings - Fork 537
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
feat: enable inline for CommonJS bundle #2442
Conversation
🦋 Changeset detectedLatest commit: f763030 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
size-limit report 📦
|
@broccolinisoup would it be possible for this to make it into the release this cycle? 🙏 |
Sure! Is there any tests on the release checklist you suggest running again or should our CI checks be enough to confirm? |
@broccolinisoup I think that if it works in the memex smoketest we should be good to go! |
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.
Seems like a good approach 👍
If the canary release works in Memex, let's go ahead and merge this 🚀
Confirming that the canary release works in memex 🥳 |
This is a follow-up to: #2404 after primer/.github#8 was merged. It restores the ability to inline ESM-only modules in the CommonJS path.
The RC workflow should now include the
lib/node_modules
directory when publishing which should remedy the underlying issue when testing out release candidates.There is an exception to the inline behavior described above with dynamic require statements. By default the
commonjs
plugin will attempt to hoist these declarations to the top-level whentransformMixedEsModules
istrue
. As a result, these statements are left as-is.There is one potential path forward with these calls where they are converted to dynamic
import()
statements in source. When they are built for ESM and CommonJS they are then converted torequire()
statements to work around a bug present in Jest and V8.Another path forward is to determine the way to make these top-level imports so that we can avoid dynamic
require()
andimport()
statements altogether.