Skip to content
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

Switch all ESM dist to .mjs #3634

Closed
wants to merge 1 commit into from
Closed

Conversation

aduh95
Copy link

@aduh95 aduh95 commented Jul 26, 2022

Jest 28 supports "exports" field in package.json, and will interpret ./dist/preact.module.js as CJS because it's using .js extension and the package.json doesn't contain "type": "module".
Switching to .mjs extension would fix the issue.

Jest 28 supports `"exports"` field in `package.json`, and will
interpret `./dist/preact.module.js` as CJS because it's using `.js`
extension and the `package.json` doesn't contain `"type": "module"`.
Switching to `.mjs` extension would fix the issue.
@@ -19,7 +19,7 @@
"exports": {
".": {
"types": "./src/index.d.ts",
"browser": "./dist/compat.module.js",
"browser": "./dist/compat.module.mjs",
"umd": "./dist/compat.umd.js",
"import": "./dist/compat.mjs",
Copy link
Member

@JoviDeCroock JoviDeCroock Jul 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we already have .mjs here, correct me if I'm wrong but apart from chromium browsers don't support the .mjs extension yet, so from what I can tell Jest should be using exports.import rather than exports.browser

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Browsers don’t care about the extension, they all support .mjs on that regard.

Jest should be using exports.import rather than exports.browser

Jest is using exports.browser when using the jest-environment-jsdom environment, you may or may not agree with that but that’s how they’re doing it 😅 anyway, Preact should be using .mjs, or add "type": "module" and use .cjs extension for CJS file, otherwise it’s hurting their users that use tooling that follow the same logic as Node.js.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it’s hurting their users that use tooling that follow the same logic as Node.js.

It's not, Node uses this correctly as it imports from import and not from browser if Jest want to import from the browser tag it feels more appropriate to start disregarding the type: module stuff and rely on the browser semantics rather than the node ones 😅

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason not to use .mjs though?

Copy link
Member

@JoviDeCroock JoviDeCroock Jul 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The thing is we could change the export map to use .mjs but we would have to leave the .module.js file intact as people could be relying on that directly (unsafe though but let's try not to break people's stuff :p). Doing some quick checks and unpkg/skypack do leverage the correct mime when serving from their CDN. As we are not touching the UMD file we can be quite sure we don't have to check if people fill in the correct mime there 😅 CC @developit @marvinhagemeister are you seeing any issues here that I am overlooking?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe the reason we didn't go with .mjs was because it forced webpack into strict module mode which was causing duplicate copies of Preact for codebases with mixed import+require. I believe that problem is now solved via the module export field, so we could move exclusively to using .mjs.

"module": "dist/preact.module.js",
"module": "dist/preact.module.mjs",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't safe to change outside of a semver major. A lot of tools/versions of tools can consume "module" but don't all support .[cm]js extensions.

A few months ago immer stumbled into an issue making this change, discovering that Metro didn't support .mjs. Only as of last month has that been rectified.

While I doubt Metro would be an issue for us, that just goes to show that support is not as widespread as we may hope. Node isn't going to consume "module" so it shouldn't need to adhere to Node's file extension semantics.

@InfiniteXyy
Copy link

I found a workaround here to use jest 28+, with preact,
You can use customExportConditions option to force jest not to resolve the "browser" field.

https://jestjs.io/docs/configuration#testenvironmentoptions-object

{
  "testEnvironmentOptions": {
    "customExportConditions": [
      "node",
      "node-addons"
    ]
  }
}

@arturi
Copy link

arturi commented Mar 30, 2023

Just faced this again when trying to use preact/hooks in a new module in uppy.io, we had to patch Preact in uppy again: transloadit/uppy@11f1476.

@rschristian
Copy link
Member

I found a workaround here to use jest 28+, with preact, You can use customExportConditions option to force jest not to resolve the "browser" field.

https://jestjs.io/docs/configuration#testenvironmentoptions-object

{
  "testEnvironmentOptions": {
    "customExportConditions": [
      "node",
      "node-addons"
    ]
  }
}

FWIW we did land this in the jest preset, but it has a rather big issue

@rschristian
Copy link
Member

Going to close this out as it's not safe for us to touch in v10 and has been addressed in (what exists of) v11

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.

6 participants