Skip to content

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

pcvera
Copy link

@pcvera pcvera commented May 2, 2025

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:

/[path]/node_modules/openapi-fetch/dist/index.js:8
export function randomID() {
^^^^^

SyntaxError: Unexpected token 'export'

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 in module, but it isn't resilient to receiving ES Module syntax from main 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 the main field:

It also defines the script that is used when the package directory is loaded via require()

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 with exports.require and make a philosophical call about whether that's consistent with their understanding of the main field in package.json. 😬 For reference, the openapi-metadata package is already aligned, but the swr-openapi package takes another strategy of specifying "type": "module" (I think).

Checklist

  • [n/a] Unit tests updated
  • [n/a] docs/ updated (if necessary)
  • [n/a] pnpm run update:examples run (only applicable for openapi-typescript)

@pcvera pcvera requested a review from a team as a code owner May 2, 2025 19:02
@pcvera pcvera requested a review from kerwanp May 2, 2025 19:02
Copy link

netlify bot commented May 2, 2025

👷 Deploy request for openapi-ts pending review.

Visit the deploys page to approve it

Name Link
🔨 Latest commit 980df59

Copy link

changeset-bot bot commented May 2, 2025

⚠️ No Changeset found

Latest commit: 980df59

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

@pcvera
Copy link
Author

pcvera commented May 2, 2025

I'm not sure what I can do about this failure - I haven't modified the build command, and the failing test requires a relative path, which isn't going to consult package.json.

Could it be a flake?

@drwpow
Copy link
Contributor

drwpow commented May 3, 2025

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 exports just fine. It’s usually Node.js that is the problem. What version of Jest / Node are you seeing a problem on?

Copy link
Contributor

@drwpow drwpow left a 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.

@drwpow drwpow self-assigned this May 3, 2025
@pcvera
Copy link
Author

pcvera commented May 6, 2025

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.

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.

2 participants