combobox-nav
combobox-nav copied to clipboard
Add a CommonJS build to the package
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 (https://github.com/primer/react/issues/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-cjsto.gitignore - Adds the
exportsobject topackage.json - Removes the
"type": "module"field frompackage.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.
the macos-latest build always fails (#58)
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 😄
Here's another downstream issue related to this (just opened):
- https://github.com/primer/react/issues/2346
I don't advocate for supporting legacy patterns, but is there any other way we can solve this?
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.
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.
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 ?