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
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions compat/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
"private": true,
"description": "A React compatibility layer for Preact",
"main": "dist/compat.js",
"module": "dist/compat.module.js",
"module": "dist/compat.module.mjs",
"umd:main": "dist/compat.umd.js",
"source": "src/index.js",
"types": "src/index.d.ts",
Expand All @@ -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.

"require": "./dist/compat.js"
Expand Down
File renamed without changes.
8 changes: 4 additions & 4 deletions config/node-13-exports.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,19 +5,19 @@ const snakeCaseToCamelCase = str =>
str.replace(/([-_][a-z])/g, group => group.toUpperCase().replace('-', ''));

const copyPreact = () => {
// Copy .module.js --> .mjs for Node 13 compat.
// Copy .module.mjs --> .mjs for Node 13 compat.
fs.writeFileSync(
`${process.cwd()}/dist/preact.mjs`,
fs.readFileSync(`${process.cwd()}/dist/preact.module.js`)
fs.readFileSync(`${process.cwd()}/dist/preact.module.mjs`)
);
};

const copy = name => {
// Copy .module.js --> .mjs for Node 13 compat.
// Copy .module.mjs --> .mjs for Node 13 compat.
const filename = name.includes('-') ? snakeCaseToCamelCase(name) : name;
fs.writeFileSync(
`${process.cwd()}/${name}/dist/${filename}.mjs`,
fs.readFileSync(`${process.cwd()}/${name}/dist/${filename}.module.js`)
fs.readFileSync(`${process.cwd()}/${name}/dist/${filename}.module.mjs`)
);
};

Expand Down
4 changes: 2 additions & 2 deletions debug/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
"private": true,
"description": "Preact extensions for development",
"main": "dist/debug.js",
"module": "dist/debug.module.js",
"module": "dist/debug.module.mjs",
"umd:main": "dist/debug.umd.js",
"source": "src/index.js",
"license": "MIT",
Expand All @@ -17,7 +17,7 @@
},
"exports": {
".": {
"browser": "./dist/debug.module.js",
"browser": "./dist/debug.module.mjs",
"umd": "./dist/debug.umd.js",
"import": "./dist/debug.mjs",
"require": "./dist/debug.js"
Expand Down
4 changes: 2 additions & 2 deletions devtools/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
"private": true,
"description": "Preact bridge for Preact devtools",
"main": "dist/devtools.js",
"module": "dist/devtools.module.js",
"module": "dist/devtools.module.mjs",
"umd:main": "dist/devtools.umd.js",
"source": "src/index.js",
"license": "MIT",
Expand All @@ -16,7 +16,7 @@
"exports": {
".": {
"types": "./src/index.d.ts",
"browser": "./dist/devtools.module.js",
"browser": "./dist/devtools.module.mjs",
"umd": "./dist/devtools.umd.js",
"import": "./dist/devtools.mjs",
"require": "./dist/devtools.js"
Expand Down
4 changes: 2 additions & 2 deletions hooks/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
"private": true,
"description": "Hook addon for Preact",
"main": "dist/hooks.js",
"module": "dist/hooks.module.js",
"module": "dist/hooks.module.mjs",
"umd:main": "dist/hooks.umd.js",
"source": "src/index.js",
"license": "MIT",
Expand All @@ -26,7 +26,7 @@
"exports": {
".": {
"types": "./src/index.d.ts",
"browser": "./dist/hooks.module.js",
"browser": "./dist/hooks.module.mjs",
"umd": "./dist/hooks.umd.js",
"import": "./dist/hooks.mjs",
"require": "./dist/hooks.js"
Expand Down
4 changes: 2 additions & 2 deletions jsx-runtime/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
"private": true,
"description": "Preact JSX runtime",
"main": "dist/jsxRuntime.js",
"module": "dist/jsxRuntime.module.js",
"module": "dist/jsxRuntime.module.mjs",
"umd:main": "dist/jsxRuntime.umd.js",
"source": "src/index.js",
"types": "src/index.d.ts",
Expand All @@ -19,7 +19,7 @@
"exports": {
".": {
"types": "./src/index.d.ts",
"browser": "./dist/jsxRuntime.module.js",
"browser": "./dist/jsxRuntime.module.mjs",
"umd": "./dist/jsxRuntime.umd.js",
"import": "./dist/jsxRuntime.mjs",
"require": "./dist/jsxRuntime.js"
Expand Down
22 changes: 11 additions & 11 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -5,61 +5,61 @@
"private": false,
"description": "Fast 3kb React-compatible Virtual DOM library.",
"main": "dist/preact.js",
"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.

"umd:main": "dist/preact.umd.js",
"unpkg": "dist/preact.min.js",
"source": "src/index.js",
"exports": {
".": {
"types": "./src/index.d.ts",
"browser": "./dist/preact.module.js",
"browser": "./dist/preact.module.mjs",
"umd": "./dist/preact.umd.js",
"import": "./dist/preact.mjs",
"require": "./dist/preact.js"
},
"./compat": {
"types": "./compat/src/index.d.ts",
"browser": "./compat/dist/compat.module.js",
"browser": "./compat/dist/compat.module.mjs",
"umd": "./compat/dist/compat.umd.js",
"import": "./compat/dist/compat.mjs",
"require": "./compat/dist/compat.js"
},
"./debug": {
"browser": "./debug/dist/debug.module.js",
"browser": "./debug/dist/debug.module.mjs",
"umd": "./debug/dist/debug.umd.js",
"import": "./debug/dist/debug.mjs",
"require": "./debug/dist/debug.js"
},
"./devtools": {
"types": "./devtools/src/index.d.ts",
"browser": "./devtools/dist/devtools.module.js",
"browser": "./devtools/dist/devtools.module.mjs",
"umd": "./devtools/dist/devtools.umd.js",
"import": "./devtools/dist/devtools.mjs",
"require": "./devtools/dist/devtools.js"
},
"./hooks": {
"types": "./hooks/src/index.d.ts",
"browser": "./hooks/dist/hooks.module.js",
"browser": "./hooks/dist/hooks.module.mjs",
"umd": "./hooks/dist/hooks.umd.js",
"import": "./hooks/dist/hooks.mjs",
"require": "./hooks/dist/hooks.js"
},
"./test-utils": {
"types": "./test-utils/src/index.d.ts",
"browser": "./test-utils/dist/testUtils.module.js",
"browser": "./test-utils/dist/testUtils.module.mjs",
"umd": "./test-utils/dist/testUtils.umd.js",
"import": "./test-utils/dist/testUtils.mjs",
"require": "./test-utils/dist/testUtils.js"
},
"./jsx-runtime": {
"types": "./jsx-runtime/src/index.d.ts",
"browser": "./jsx-runtime/dist/jsxRuntime.module.js",
"browser": "./jsx-runtime/dist/jsxRuntime.module.mjs",
"umd": "./jsx-runtime/dist/jsxRuntime.umd.js",
"import": "./jsx-runtime/dist/jsxRuntime.mjs",
"require": "./jsx-runtime/dist/jsxRuntime.js"
},
"./jsx-dev-runtime": {
"browser": "./jsx-runtime/dist/jsxRuntime.module.js",
"browser": "./jsx-runtime/dist/jsxRuntime.module.mjs",
"umd": "./jsx-runtime/dist/jsxRuntime.umd.js",
"import": "./jsx-runtime/dist/jsxRuntime.mjs",
"require": "./jsx-runtime/dist/jsxRuntime.js"
Expand All @@ -69,7 +69,7 @@
"require": "./compat/client.js"
},
"./compat/server": {
"browser": "./compat/server.browser.js",
"browser": "./compat/server.browser.mjs",
"import": "./compat/server.mjs",
"require": "./compat/server.js"
},
Expand Down Expand Up @@ -203,7 +203,7 @@
"compat/src",
"compat/client.js",
"compat/client.mjs",
"compat/server.browser.js",
"compat/server.browser.mjs",
"compat/server.js",
"compat/server.mjs",
"compat/scheduler.js",
Expand Down
4 changes: 2 additions & 2 deletions test-utils/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
"private": true,
"description": "Test-utils for Preact",
"main": "dist/testUtils.js",
"module": "dist/testUtils.module.js",
"module": "dist/testUtils.module.mjs",
"umd:main": "dist/testUtils.umd.js",
"source": "src/index.js",
"license": "MIT",
Expand All @@ -19,7 +19,7 @@
"exports": {
".": {
"types": "./src/index.d.ts",
"browser": "./dist/testUtils.module.js",
"browser": "./dist/testUtils.module.mjs",
"umd": "./dist/testUtils.umd.js",
"import": "./dist/testUtils.mjs",
"require": "./dist/testUtils.js"
Expand Down