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

docs: correct the Node.js version base #13099

Merged
merged 2 commits into from Oct 2, 2021

Conversation

yorkie
Copy link
Contributor

@yorkie yorkie commented Sep 22, 2021

Summary

The PR(#13045) converts cli to use esm which also uses named exports for commonjs, this commit just corrects the description in README.

Related Issues/PRs

The PR#13045 converts cli to use esm which also uses named exports for
commonjs, this commit just corrects the description in README.
@google-cla google-cla bot added the cla: yes label Sep 22, 2021
@yorkie yorkie changed the title readme: correct the Node.js version base docs: correct the Node.js version base Sep 22, 2021
@yorkie yorkie marked this pull request as ready for review September 22, 2021 15:48
@yorkie yorkie requested a review from a team as a code owner September 22, 2021 15:48
@yorkie yorkie requested review from adamraine and removed request for a team September 22, 2021 15:48
@yorkie
Copy link
Contributor Author

yorkie commented Sep 22, 2021

See https://twitter.com/MylesBorins/status/1311033983824793601, this is a tweet about named imports from CJS.

@connorjclark
Copy link
Collaborator

connorjclark commented Sep 22, 2021

12.13 (as defined in our package.json) does indeed error.

12.17.0 is earliest version that does not require the experimental modules flag:

https://github.com/nodejs/node/blob/master/doc/changelogs/CHANGELOG_V12.md#2020-05-26-version-12170-erbium-lts-targos

Can you change the version here + in package.json to be 12.17.0? We'll be moving to Node 14 in ~month for LH v9, but can't do it yet as it would be a breaking change.

@yorkie
Copy link
Contributor Author

yorkie commented Sep 22, 2021

12.13 (as defined in our package.json) does indeed error.

12.17.0 is earliest version that does not require the experimental modules flag:

@connorjclark, thanks for your reply, and it seems here to be 12.20.0 because the named imports are not supported yet at 12.17.0, and I also reproduce the issue at this version at my MBP, and it gets fixed when I used 12.20.0.

BTW the correct versions are >= 12.20 for Node12 and >=14.13 for Node14 via nodejs/node#35249, do you have any suggestion?

@connorjclark
Copy link
Collaborator

connorjclark commented Sep 22, 2021

ah ok, let's list the minimum versions for 12 as 12.20 and 14 as 14.13 in the readme then. (and 12.20 for package.json)

I tried different variations of compound version checks in the engines.node field (and made a .npmrc of engine-strict=true) but npm doesn't seem to support it (and has no docs suggesting they support anything complex) :/ So we're forced to just do >=12.20.0.

EDIT:

actually >=12.20.0 && 12.* || >=14.13 && 14.* || >=15 seems to work in the engine field.

@yorkie
Copy link
Contributor Author

yorkie commented Sep 23, 2021

@connorjclark Updated.

@yorkie
Copy link
Contributor Author

yorkie commented Sep 26, 2021

@connorjclark May I ask you to have a look?

@Mbellsudteen
Copy link

Mbellsudteen commented Oct 2, 2021 via email

@Mbellsudteen
Copy link

Mbellsudteen commented Oct 2, 2021 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants