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

fix(create): load entry file after node version checking #6860

Merged
merged 2 commits into from
Mar 6, 2022

Conversation

taejs
Copy link
Contributor

@taejs taejs commented Mar 6, 2022

Motivation

Fix #6843
Imports are hoisted in ESM environment. Are ES6 module imports hoisted?
So before version checking, the interpreter reads index.js and fails when it's an unsupported node version.
Using ESM dynamic import, I changed the order.

Additionally, I just refactored the error handling code refers to 85e87b5

Have you read the Contributing Guidelines on pull requests?

Changes

  • AS IS

node v12.22

$ npx create-docusaurus
file:///Users/user/Desktop/docusaurus/packages/create-docusaurus/lib/index.js:36
    return PackageManagersList.find((packageManager) => process.env.npm_config_user_agent?.startsWith(packageManager));
                                                                                          ^
SyntaxError: Unexpected token '.'
    at Loader.moduleStrategy (internal/modules/esm/translators.js:140:18)
  • TO BE

node v12.22

$ npx create-docusaurus
[ERROR] Minimum Node.js version not met :(
[INFO] You are using Node.js v12.22.3, Requirement: Node.js >=14.

Test Plan

Related PRs

@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Mar 6, 2022
Copy link
Collaborator

@Josh-Cena Josh-Cena left a comment

Choose a reason for hiding this comment

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

Thanks, that looks great!

But just confirming, this would still be parser error if we have incompatible syntax in bin/index.js, or if the user uses a super-old Node like v10 without ESM, right?

@netlify
Copy link

netlify bot commented Mar 6, 2022

✔️ [V2]
Built without sensitive environment variables

🔨 Explore the source changes: ab19dd8

🔍 Inspect the deploy log: https://app.netlify.com/sites/docusaurus-2/deploys/6224b8b3923a0500070b8e78

😎 Browse the preview: https://deploy-preview-6860--docusaurus-2.netlify.app

@github-actions
Copy link

github-actions bot commented Mar 6, 2022

⚡️ Lighthouse report for the changes in this PR:

Category Score
🟠 Performance 53
🟢 Accessibility 100
🟢 Best practices 92
🟢 SEO 100
🟢 PWA 90

Lighthouse ran on https://deploy-preview-6860--docusaurus-2.netlify.app/

@Josh-Cena Josh-Cena added the pr: polish This PR adds a very minor behavior improvement that users will enjoy. label Mar 6, 2022
@taejs
Copy link
Contributor Author

taejs commented Mar 6, 2022

You're right. But under 12 is not maintaining any more, so I think that support super-old versions is low priority.
After deprecation of version 12 - also we can remove this code.

image

@Josh-Cena
Copy link
Collaborator

Agreed. It's just that we want to give nicer error messages than a parser failure :) I'm surprised that we can't configure the package manager to panic in this case. (And from my conversation with Yarn maintainers, seems they removed the engines check in Yarn berry altogether.)

@taejs
Copy link
Contributor Author

taejs commented Mar 6, 2022

In case of create-react-app, it works because it's using CJS module system. 🥺
https://github.com/facebook/create-react-app/blob/main/packages/create-react-app/index.js

@Josh-Cena
Copy link
Collaborator

Josh-Cena commented Mar 6, 2022

I see. Technically, we can change that require to an equivalent await import(), because we are ESM and we can use top-level await :D Do you want to do that change?

@taejs
Copy link
Contributor Author

taejs commented Mar 6, 2022

I've tried, but top-level await is supported after v14.8.
In node 12, you can see this error when we use await import.

$ npx create-docusaurus
file:///Users/user/Desktop/docusaurus/packages/create-docusaurus/bin/index.js:28
const init = await import(`../lib/index.js`);
             ^^^^^

SyntaxError: Unexpected reserved word
    at Loader.moduleStrategy (internal/modules/esm/translators.js:140:18)
    at async link (internal/modules/esm/module_job.js:42:21)

@Josh-Cena
Copy link
Collaborator

Oh, I see. Let's leave it like this then! Thanks!

@Josh-Cena Josh-Cena merged commit a6e7219 into facebook:main Mar 6, 2022
@slorber
Copy link
Collaborator

slorber commented Mar 9, 2022

Nice trick, seems to work fine :) thanks

image

@aafnnp
Copy link

aafnnp commented May 18, 2022

how to resolve this problem below v14.8

@Josh-Cena
Copy link
Collaborator

@Manonicu We now require v14.13+ (given in the installation docs). Bumping minor Node versions is usually not seen as a problem, so we won't specify it explicitly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Signed Facebook CLA pr: polish This PR adds a very minor behavior improvement that users will enjoy.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

npx create-docusaurus does not check engines field
5 participants