-
-
Notifications
You must be signed in to change notification settings - Fork 532
Align package.json main
fields to point to CommonJS bundles
#2269
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
base: main
Are you sure you want to change the base?
Conversation
👷 Deploy request for openapi-ts pending review.Visit the deploys page to approve it
|
|
I'm not sure what I can do about this failure - I haven't modified the build command, and the failing test Could it be a flake? |
Thanks for raising! While I don’t mind improving support for older systems, this is too big of a change (technically this is a breaking change, and would require major version bumps). To my knowledge, Jest should respect |
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.
Just marking “request changes” for now since this is an undiscussed change that could have big consequences. That doesn’t mean I’m opposed to this landing, I just wanted to make clear to the other maintainers some alignment is still needed here.
It took some investigation, but it looks like our tooling calls into 27.5.1, which is actually more recent than I would have expected. |
Hello!
This PR targets backwards-compatibility for these packages. We found that, in my work's legacy build system, Jest falls down on the
openapi-fetch
package because of the reported syntax error:This is a symptom of Jest expecting to find CommonJS syntax and unexpectedly finding ES Module sytanx.
Jest can handle CommonJS when specified in
main
or ES Module syntax when specified inmodule
, but it isn't resilient to receiving ES Module syntax frommain
where its expecting CommonJS.The goal for this change is to improve backwards compatibility with legacy resolution. Most more recent systems will prefer the paths in
exports
, which all of these packages already have specified.There are other options for improving the legacy Jest case, but this is the lowest-churn way, without changing build systems or holistically modifying resolution for the package.
Caveat
I'm not certain this is an official standard - that
main
corresponds to CJS. It's hard to find concrete recommendations saying this directly, the closest thing I can find is that the Node JS documentation says about themain
field:where
require
is a CommonJS syntax.reference
Changes
Align the existing
main
fields in this repository's package.json files on their CommonJS bundles.How to Review
How can a reviewer review your changes? What should be kept in mind for this review?
Confirm the
main
fields are aligned withexports.require
and make a philosophical call about whether that's consistent with their understanding of themain
field in package.json. 😬 For reference, theopenapi-metadata
package is already aligned, but theswr-openapi
package takes another strategy of specifying"type": "module"
(I think).Checklist
docs/
updated (if necessary)pnpm run update:examples
run (only applicable for openapi-typescript)