Update "exports" to use the correct conditions

[DJMcNab]

From the npm docs:

"node" - matches for any Node.js environment. Can be a CommonJS or ES module file. In most cases explicitly calling out the Node.js platform is not necessary.

At the moment, node environments unconditionally use the require module type in lib. However, this module type is broken because of #2245. This change allows using the ES modules version for node environments instead, when specified using import.

Note that this doesn't fix the issue causing #2245 - importing "@primer/react" using require will still be broken. However, this does allow this module to work with nodejs (when using import), including next.js.

Merge checklist

I'm not sure any of these are applicable, but I've left them pending further guidance.

• Added/updated tests
• Added/updated documentation
• Tested in Chrome
• Tested in Firefox
• Tested in Safari
• Tested in Edge

Take a look at the What we look for in reviews section of the contributing guidelines for more information on how we review PRs.

[okuryu]

JSON doesn't allow a trailing comma.

[DJMcNab]

Ah, the danger of trusting myself with GitHub's web editor.

Thanks, I'll get that sorted.

[pksjce]

I like this change. It makes the exports more specific to the environment. Happy to merge when CI passes.

[changeset-bot]

⚠️ No Changeset found

Latest commit: 6b5c0d1

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[DJMcNab]

I think this does need to result in a release of this package, so I guess we do need a changeset.

I don't believe this package's API includes it being imported matching neither of given conditions, so I don't believe this is to be a breaking change. Although I don't think there's any harm in marking this as a breaking change to be safe. I'll be happy to update either way, but I'm also happy for someone else to add the changeset once a consensus is reached.

[pksjce]

@DJMcNab - Do you think removing the "default" option bite some consumer in some way or cause a regression? All implementations will fall in either "require" or "import" right?

[DJMcNab]

No, I don't believe it will cause a regression.

Any JavaScript runtime will match one of the two conditions, so it would have to be a non JavaScript consumer. And I can't think of a single possible case where it would be needed, since this is a JavaScript library.

[pksjce]

I tried your PR on a nextjs app setup with SSR. I still get this error.

But its supposed to get fixed with this change right?

[DJMcNab]

I was finally able to get a local development version of this working - I had been having issues with virtualisation, so couldn't actually build this locally until I got that sorted.

We appear to be running into vercel/next.js#37419 now. I could resolve that locally by setting this module to "type": "module". Alternatively, I think setting experimental.esmExternals in the next config to "loose" also fixes this (I'm not sure if this PR is required in that case?)

However, after this I ran into another issue, where webpack dislikes the dynamism in styled-components. (this appears to be styled-components/styled-components#3601 )

Overall, I'm a bit out of my depth here I'm afraid. Surprisingly, webpack seems to either have no escape hatch for dynamic fields on the default export, or has extremely poor discoverability for that escape hatch. It appears that setting usedExports to false might fix this, but at this point I'm not sure I have the will to dig into this much further.

One other thing I did find is that in a project without this PR, just importing the specific files directly works. e.g.

import BaseStyles from "@primer/react/lib-esm/BaseStyles";

Note that it needs to be lib-esm for typescript to pick it up properly.
This is a good enough workaround for me, until either this package becomes ESM only, and/or next.js sorts itself out.
I'm not sure why this doesn't run into styled-components/styled-components#3601