Add a CommonJS build to the package

[iansan5653]

The lack of a CommonJS build is preventing this dependency from being used in applications using NextJS. By nature of @primer/react depending on this, that means @primer/react also cannot be used with NextJS (primer/react#2245). To fix this, we need to add the the CommonJS build and configure package.json such that dependents will be able to locate the correct file.

This PR:

• Adds a CommonJS build outputted to /dist-cjs
• Adds /dist-cjs to .gitignore
• Adds the exports object to package.json
• Removes the "type": "module" field from package.json (not 100% sure on this one)

I don't have a good test environment setup for this unfortunately, so please do take a close look. I think this is right, based on looking at the build output, but I'm not completely confident.

[iansan5653]

the macos-latest build always fails (#58)

[mattcosta7]

Hm - I don't have a good thought for how we could best add a test scenario for this, but it would be nice if we could validate this works for node type requires as well.

[mattcosta7]

curios if we want to remove this, or if renaming dist-cjs/index.js to dist/cjs/index.cjs might b another option here.

[iansan5653]

Renaming might be an option, but I don't see a way to do that with tsc and I'd rather not introduce another tool. I saw that primer/react doesn't have this field so I figured it's probably okay to remove it.

[koddsson]

I don't think we want to do this. CommonJS is a legacy pattern and we don't want to support it. As far as I can tell NextJS already supports ES Modules so I feel like I'm missing something 😄

[iansan5653]

Here's another downstream issue related to this (just opened):

• Test error on @primer/react/lib/drafts/hooks/useCombobox.js primer/react#2346

I don't advocate for supporting legacy patterns, but is there any other way we can solve this?

[koddsson]

Given the assertion:

The lack of a CommonJS build is preventing this dependency from being used in applications using NextJS.

And the fact that NextJS does support ESModules I'm unsure what the actual issue is that we are trying to solve by adding CommonJS builds to combobox-nav.

[iansan5653]

While Next does now support ESM, I believe migrating requires users to change all of their require statements to import statements, at which point I am sure they will run into more broken dependencies. While that's a good direction to move in, perhaps the ecosystem is not completely ready for ESM everywhere yet.

Honestly I don't know if this is the best way to go 🤷. I'm not an expert in this area and this problem doesn't affect any internal projects that I know of. I don't feel extremely strongly one way or the other - I just figured this was the easiest solution to avoid breaking changes for consumers.

[mattcosta7]

I don't think we want to do this. CommonJS is a legacy pattern and we don't want to support it. As far as I can tell NextJS already supports ES Modules so I feel like I'm missing something 😄

We could attempt to move primer's builds to only ESM, which might avoid this

For clarity, the issue is not directly with how combobox-nav is used, but by how primer/react transforms the usage, when that library attempts to build for cjs and esm.

the code generated in the primer/react build process transforms the import of combobox-nav to require('combobox-nav') and naturally, requiring this is not possible since there's no CJS build to require

Supporting that here, avoids the need for a breaking change on primer that drops CJS support entirely - but maybe the pushback is really, primer/react needs to just go all in ?