feat: support `"type": "module"` #485

tmm · 2022-07-19T19:15:51Z

Adds support for ESM-only builds when "type": "module" is added to package.json.

Add support to init, validate, fix, dev, and build commands
Update and add tests
Update docs for "type": "module"
Add changeset for release

Closes #471

Happy to chat about this more or modify the approach. Some open questions:

Should this get added behind an experimental flag?
What version bump does this change receive?

changeset-bot · 2022-07-19T19:15:53Z

⚠️ No Changeset found

Latest commit: 04b8d32

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

codecov · 2022-07-19T19:19:46Z

Codecov Report

Merging #485 (04b8d32) into main (0c79455) will increase coverage by 0.10%.
The diff coverage is 97.91%.

@@            Coverage Diff             @@
##             main     #485      +/-   ##
==========================================
+ Coverage   92.14%   92.25%   +0.10%     
==========================================
  Files          32       32              
  Lines        1452     1484      +32     
  Branches      395      413      +18     
==========================================
+ Hits         1338     1369      +31     
- Misses        108      109       +1     
  Partials        6        6

Impacted Files	Coverage Δ
packages/cli/src/entrypoint.ts	`100.00% <ø> (ø)`
packages/cli/src/messages.ts	`83.33% <ø> (ø)`
packages/cli/src/init.ts	`80.00% <85.71%> (ø)`
packages/cli/src/build/config.ts	`83.78% <100.00%> (+1.43%)`	⬆️
packages/cli/src/dev.ts	`97.26% <100.00%> (+0.07%)`	⬆️
packages/cli/src/package.ts	`95.42% <100.00%> (+0.15%)`	⬆️
packages/cli/src/utils.ts	`100.00% <100.00%> (ø)`
packages/cli/src/validate-package.ts	`95.58% <100.00%> (+0.13%)`	⬆️
packages/cli/src/validate.ts	`91.76% <100.00%> (+0.30%)`	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

emmatown · 2022-07-26T03:32:48Z

Hey, thanks for opening this PR

I’ve been thinking about what exactly Node ESM support should like in Preconstruct for a few years now and I’m still not sure what exactly the right answer is but I think it’d be worth trying something and to start with, I’m thinking “let’s only support all the new things and do things “correctly” and see how painful it is” would be an interesting starting place so I’ve pushed some changes to the documentation with the behavior I’m imagining. Let me know what you think. Apologies that this isn’t exactly a quick fix.

cc @Andarist curious if you have any thoughts on this

Andarist · 2022-07-26T10:31:36Z

packages/cli/src/__tests__/fix.ts

+    "one/package.json": JSON.stringify({}),
+    "two/package.json": JSON.stringify({}),


It's probably a safe bet to assume that if a tool understands type: 'module' then it also understands the package.json#exports field. So assuming that, we could stop generating nested package.json directories for entrypoints and rely on package.json#exports entirely for entrypoints.

Andarist · 2022-07-26T10:35:22Z

packages/cli/src/build/config.ts

-    ],
-  });
+  if (!hasModuleType)
+    configs.push({


Those files were also acting as optimizations for node since reading from process.env is slow. In ESM-only world, we should probably leverage development/production conditions in package.json#exports to apply the same optimization. I know that @mitchellhamilton had some concerns related to that though

Good with adding this in. @mitchellhamilton any thoughts?

I think it should be done as a separate change to this. The big thing IMO is that the syntax for it shouldn't be process.env.NODE_ENV since you could have process.env.NODE_ENV === 'production' but not have the production condition set.

Andarist · 2022-07-26T10:37:27Z

packages/cli/src/dev.ts

-            ),
-          ];
-          if (entrypoint.json.module) {
+          if (entrypoint.json.module || entrypoint.json.type === "module") {


general question - should we even bother supporting "mixed" packages where some entrypoints are of type module and some are of type commonjs? IMHO the added complexity isn't quite worth it

packages/cli/src/messages.ts

Andarist · 2022-07-26T10:44:03Z

site/docs/configuration.md

+"module" | "commonjs";
+```
+
+> If you're just thinking "I want to write code with the native ECMAScript import and export syntax and have my code work for most people", you likely should not use this feature. This feature allows you to build packages that import Node.js ESM only dependencies and therefore only support being imported from ESM in newer versions of Node.js and some bundlers.


OTOH, it's not that uncommong to have self-contained repositories without external dependencies. In such it should be possible to use this feature without any "fear" and without deeper understanding of things. It's a fine line though and once you opt into this then you might end up with problems in the future, once you start adding external deps 🤷

It's also not completely true that all deps have to be "esm only". It's very nuanced though and I'm not sure how to touch on that subject here, maybe we should link to a further explanation that could be put somewhere below? (you already touch the subject here)

site/docs/configuration.md

Andarist · 2022-07-26T10:47:53Z

site/docs/configuration.md

+
+- The [`exports` field feature](#exports) must also be enabled.
+- There are no entrypoint `package.json`s, the entrypoints are only specified in the `exports` field
+- The dist files for all entrypoints are in the `dist` directory in the root of the package


This could be true regardless of this new option. Could we align the behavior for regular builds?

Yeah, I was thinking that as well

Andarist · 2022-07-26T10:48:37Z

site/docs/configuration.md

+- The [`exports` field feature](#exports) must also be enabled.
+- There are no entrypoint `package.json`s, the entrypoints are only specified in the `exports` field
+- The dist files for all entrypoints are in the `dist` directory in the root of the package
+- No `main`, `module`, `browser` or `umd:main` fields are used


might be worth mentioning the available conditions here

Andarist · 2022-07-26T10:50:21Z

site/docs/configuration.md

+- The dist files for all entrypoints are in the `dist` directory in the root of the package
+- No `main`, `module`, `browser` or `umd:main` fields are used
+- Module resolution internally follows [Node.js ESM module resolution](https://nodejs.org/api/esm.html#import-specifiers) which requires explicitly writing extensions in imports. This includes following `"moduleResolution": "nodenext"` in TypeScript which means writing `.js` to import a TypeScript file.
+- The default export of a CommonJS dependency(even if it provides ESM intended for bundlers) will be the whole exports object, not `exports.default` if `exports.__esModule` is set and otherwise the whole exports object which is the default behaviour in Preconstruct.


This could be rephrased somehow as it's hard to read and grok. I understand how it works but not sure if I would be able to fully understand it after reading just this point

Andarist · 2022-07-26T10:51:08Z

Yea, I think that "use it at your own risk" is a good strategy and a way for us to test things out.

tmm · 2022-07-29T20:15:14Z

@mitchellhamilton @Andarist thanks for the thoughts! Cool if I make some updates that better match the updated docs and your feedback?

Andarist · 2022-07-29T21:00:43Z

I would say - go for it 😉

emmatown · 2022-07-29T21:41:01Z

yep!

emmatown · 2022-08-10T10:20:45Z

Just want to note that microsoft/TypeScript#50152 might affect what we want to do here

Andarist · 2022-08-10T10:27:06Z

An explicit types condition for each package export should always work though, right? I don't imagine a world in which it wouldn't - that would be madness 🤣

emmatown · 2022-08-10T23:36:16Z

Yeah, the design documented here would still work under "moduleResolution": "node16". One part of that issue though is essentially saying "unless people are actually using Node for module resolution (which a lot of the time, they are not) "moduleResolution": "node16" might be the wrong choice" so Preconstruct building packages that only work with "moduleResolution": "node16" might not make sense.

Co-authored-by: Mateusz Burzyński <mateuszburzynski@gmail.com>

tmm added 3 commits July 19, 2022 11:56

feat: add esm support to validate, build, dev

ad5cebf

feat: add esm support to init and fix

08f3ce6

docs: add module type

2052127

Merge branch 'main' into tmm/esm-support

f9db23e

Docs changes

9d86486

emmatown added 2 commits July 26, 2022 13:41

Update configuration.md

3a73306

Update a thing

2666d1a

Andarist reviewed Jul 26, 2022

View reviewed changes

packages/cli/src/messages.ts Outdated Show resolved Hide resolved

Andarist reviewed Jul 26, 2022

View reviewed changes

site/docs/configuration.md Outdated Show resolved Hide resolved

Andarist reviewed Jul 26, 2022

View reviewed changes

site/docs/configuration.md Outdated Show resolved Hide resolved

Andarist reviewed Jul 26, 2022

View reviewed changes

Update site/docs/configuration.md

b1769d2

tmm added 2 commits August 13, 2022 18:21

Merge branch 'main' into tmm/esm-support

9cbfc34

test: fix based on main changes

e2b6722

tmm and others added 3 commits August 13, 2022 18:59

chore: update module field error message

b93266b

Co-authored-by: Mateusz Burzyński <mateuszburzynski@gmail.com>

docs: update module type description

08918b0

Co-authored-by: Mateusz Burzyński <mateuszburzynski@gmail.com>

test: update snapshots

04b8d32

tmm closed this by deleting the head repository Nov 12, 2022

Yuripetusko mentioned this pull request Dec 26, 2022

Error with type: module #507

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: support `"type": "module"` #485

feat: support `"type": "module"` #485

tmm commented Jul 19, 2022 •

edited

changeset-bot bot commented Jul 19, 2022 •

edited

codecov bot commented Jul 19, 2022 •

edited

emmatown commented Jul 26, 2022

Andarist Jul 26, 2022

Andarist Jul 26, 2022

tmm Aug 13, 2022

emmatown Aug 16, 2022

Andarist Jul 26, 2022

Andarist Jul 26, 2022

Andarist Jul 26, 2022

Andarist Jul 26, 2022

emmatown Jul 26, 2022

Andarist Jul 26, 2022

Andarist Jul 26, 2022

Andarist commented Jul 26, 2022

tmm commented Jul 29, 2022

Andarist commented Jul 29, 2022

emmatown commented Jul 29, 2022

emmatown commented Aug 10, 2022

Andarist commented Aug 10, 2022

emmatown commented Aug 10, 2022 •

edited

		"one/package.json": JSON.stringify({}),
		"two/package.json": JSON.stringify({}),

feat: support "type": "module" #485

feat: support "type": "module" #485

Conversation

tmm commented Jul 19, 2022 • edited

changeset-bot bot commented Jul 19, 2022 • edited

⚠️ No Changeset found

codecov bot commented Jul 19, 2022 • edited

Codecov Report

emmatown commented Jul 26, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Andarist commented Jul 26, 2022

tmm commented Jul 29, 2022

Andarist commented Jul 29, 2022

emmatown commented Jul 29, 2022

emmatown commented Aug 10, 2022

Andarist commented Aug 10, 2022

emmatown commented Aug 10, 2022 • edited

feat: support `"type": "module"` #485

feat: support `"type": "module"` #485

tmm commented Jul 19, 2022 •

edited

changeset-bot bot commented Jul 19, 2022 •

edited

codecov bot commented Jul 19, 2022 •

edited

emmatown commented Aug 10, 2022 •

edited