-
Notifications
You must be signed in to change notification settings - Fork 538
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
fix(package): update exports to import/require #2366
Conversation
🦋 Changeset detectedLatest commit: cfb54c1 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 📦
|
I think all these pull requests should be settled first or closed |
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.
This seems like a great incremental step. Ship it! 🚢
Update the conditional exports used for the entrypoints of
@primer/react
fromnode
,require
, anddefault
torequire
andimport
.In the current setup,
node
will match to CommonJS which can cause issues with frameworks like Next.js which can handle ESM directly. In the proposed setup, the CommonJS path will be brought in when the package is imported throughrequire()
. The ESM path will be brought in when the package is imported usingimport from
orimport()
.This PR also addresses issues in the CommonJS bundle by inlining ESM-only dependencies. This is intended as an interim step to get this bundle working again and is not intended as a long-term solution.
Testing
Included in this PR is a Next.js example under
examples/nextjs
. This example can be used to see the difference in theexports
field in order to demonstrate that@primer/react
can participate in server-side rendering.This will not include all exports as some still may reference DOM globals and for which a separate test will need to be introduced.
To see how the export changes work, you can:
package.json
"."
export value, runnpm run develop
inexamples/nextjs
to verify that Next.js tries to useexports
to lookup the entrypoint for@primer/react
(it should fail to find it)"."
export value, remove the"import"
key, runnpm run develop
inexamples/nextjs
to verify that Next.js tries to lookup theimport
value (it should fail to load path ".")"import"
key, runnpm run develop
, and verify that Next.js can compile (you can also remove"require"
to double-check that Next.js is not going down that path)