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

fix(package): update exports to import/require #2366

Merged
merged 11 commits into from
Sep 28, 2022
Merged

Conversation

joshblack
Copy link
Member

@joshblack joshblack commented Sep 23, 2022

Update the conditional exports used for the entrypoints of @primer/react from node, require, and default to require and import.

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 through require(). The ESM path will be brought in when the package is imported using import from or import().

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 the exports 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:

  • Visit package.json
  • Remove the "." export value, run npm run develop in examples/nextjs to verify that Next.js tries to use exports to lookup the entrypoint for @primer/react (it should fail to find it)
  • Restore the "." export value, remove the "import" key, run npm run develop in examples/nextjs to verify that Next.js tries to lookup the import value (it should fail to load path ".")
  • Restore the "import" key, run npm 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)

@changeset-bot
Copy link

changeset-bot bot commented Sep 23, 2022

🦋 Changeset detected

Latest commit: cfb54c1

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@primer/react Minor

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

@github-actions
Copy link
Contributor

github-actions bot commented Sep 23, 2022

size-limit report 📦

Path Size
dist/browser.esm.js 77.1 KB (0%)
dist/browser.umd.js 77.71 KB (0%)

@joshblack joshblack temporarily deployed to github-pages September 23, 2022 16:11 Inactive
@joshblack joshblack temporarily deployed to github-pages September 23, 2022 16:35 Inactive
@joshblack joshblack temporarily deployed to github-pages September 23, 2022 16:44 Inactive
@joshblack joshblack marked this pull request as ready for review September 23, 2022 17:50
@joshblack joshblack requested review from a team and siddharthkp September 23, 2022 17:50
@joshblack joshblack temporarily deployed to github-pages September 23, 2022 17:57 Inactive
@joshblack joshblack temporarily deployed to github-pages September 23, 2022 18:13 Inactive
@Ephantuz
Copy link

I think all these pull requests should be settled first or closed

Copy link
Contributor

@colebemis colebemis left a 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! 🚢

@joshblack joshblack temporarily deployed to github-pages September 28, 2022 17:07 Inactive
@joshblack joshblack merged commit 13b4460 into main Sep 28, 2022
@joshblack joshblack deleted the fix/update-exports branch September 28, 2022 17:12
@primer-css primer-css mentioned this pull request Sep 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants