Skip to content

Conversation

tats-u
Copy link
Contributor

@tats-u tats-u commented Jan 2, 2025

Description

Pros:

  • The extra process in index.cjs will be skipped for Node 22.12 or later or bundlers supporting sync ESMs.
  • Can be clarify that Prettier can be require(ESM)ed because it doesn't contain top-level awaits.

Cons:

The following unpleasant warning will be out when we run require("prettier"): Only 22.12.x

(node:23928) ExperimentalWarning: The REPL is loading ES Module I:\prettier\src\index.js using require().
Support for loading ES Module in require() is an experimental feature and might change at any time
(Use `node --trace-warnings ...` to show where the warning was created)

We may not be possible to accept this PR now due to this.
If require(ESM) is stable in the future (Node 24?), we will be able to merge without hesitation.
The warning is not shown since 22.13, so we don't have to care about this now.

Checklist

  • I’ve added tests to confirm my change works.
  • (If changing the API or CLI) I’ve documented the changes I’ve made (in the docs/ directory).
  • (If the change is user-facing) I’ve added my changes to changelog_unreleased/*/XXXX.md file following changelog_unreleased/TEMPLATE.md.
  • I’ve read the contributing guidelines.

Try the playground for this PR

@tats-u tats-u marked this pull request as draft January 2, 2025 13:55
Copy link

pkg-pr-new bot commented Jan 2, 2025

Open in Stackblitz

npm i https://pkg.pr.new/prettier@16958

commit: e792b7d

@tats-u
Copy link
Contributor Author

tats-u commented Jan 2, 2025

Should I add Markdown files in docs/ or changelog_unreleased/?

@fisker
Copy link
Member

fisker commented Jan 3, 2025

Should I add Markdown files in docs/ or changelog_unreleased/?

changelog_unreleased

@fisker
Copy link
Member

fisker commented Jan 3, 2025

We may not be possible to accept this PR now due to this.

I think it's fine, it won't break require(cjs) if old version used, right?

@@ -15,6 +15,7 @@
"exports": {
".": {
"types": "./src/index.d.ts",
"module-sync": "./src/index.js",
Copy link
Member

Choose a reason for hiding this comment

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

The actual exports

@tats-u
Copy link
Contributor Author

tats-u commented Jan 5, 2025

I think it's fine, it won't break require(cjs) if old version used, right?

Exactly I think. Old versions can't recognize module-sync.

@tats-u tats-u marked this pull request as ready for review January 5, 2025 11:07
@stianjensen
Copy link

Node 22.13, released today, removed the warning:

https://nodejs.org/en/blog/release/v22.13.0

nodejs/node#56194

@fisker
Copy link
Member

fisker commented Jan 8, 2025

Do you think we should also add module-sync to plugin entries?

@tats-u
Copy link
Contributor Author

tats-u commented Jan 8, 2025

Optional (low priority) unless users require() them in their CJS code or the REPL.

Probably preferred

@tats-u
Copy link
Contributor Author

tats-u commented Jan 8, 2025

Added a changelog reflecting the change in Node 22.13.

@tats-u

This comment has been minimized.

@fisker
Copy link
Member

fisker commented Jan 8, 2025

I think you install dist somewhere clean to test

require("prettier/index.cjs") === require("prettier")
require("prettier/index.mjs") === require("prettier")

I don't think require(dir) reads packages.json

@tats-u
Copy link
Contributor Author

tats-u commented Jan 9, 2025

It works after npm link path/to/prettier/dist from a newly created independent package:

$ npm init
This utility will walk you through creating a package.json file.
It only covers the most common items, and tries to guess sensible defaults.

See `npm help init` for definitive documentation on these fields
and exactly what they do.

Use `npm install <pkg>` afterwards to install a package and
save it as a dependency in the package.json file.

Press ^C at any time to quit.
package name: (prettier-test)
version: (1.0.0)
description:
entry point: (index.js)
test command:
git repository:
keywords:
author:
license: (ISC)
About to write to i:\prettier-test\package.json:

{
  "name": "prettier-test",
  "version": "1.0.0",
  "description": "",
  "main": "index.js",
  "scripts": {
    "test": "echo \"Error: no test specified\" && exit 1"
  },
  "author": "",
  "license": "ISC"
}


Is this OK? (yes) yes
$ npm link ../prettier/dist

added 1 package, and audited 3 packages in 1s

1 package is looking for funding
  run `npm fund` for details

found 0 vulnerabilities
$ node
Welcome to Node.js v22.13.0.
Type ".help" for more information.
> require("prettier").__esModule
true
> require("prettier") === require("prettier/index.mjs")
true
> require("prettier") === require("prettier/index.cjs")
false
>

Node 20 can require(ESM) under a flag
Co-authored-by: fisker Cheung <lionkay@gmail.com>
@tats-u tats-u changed the title Use ESM entrypoint for require if require(ESM) is enabled Use ESM entrypoint for require(ESM) Jan 9, 2025
@tats-u
Copy link
Contributor Author

tats-u commented Jan 13, 2025

Is this included in 3.5 or 4.0?

@fisker fisker added this to the 3.5 milestone Jan 16, 2025
@fisker fisker requested a review from sosukesuzuki January 20, 2025 06:26
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.

4 participants