Skip to content

Commit

Permalink
Update build requirements in CONTRIBUTING.md (#12766)
Browse files Browse the repository at this point in the history
  • Loading branch information
@sosukesuzuki
sosukesuzuki committed Feb 5, 2021
1 parent bf523da commit 87f264c
Showing 1 changed file with 1 addition and 1 deletion.
2 changes: 1 addition & 1 deletion CONTRIBUTING.md
View file
Expand Up @@ -39,7 +39,7 @@ Feel free to check out the `#discussion`/`#development` channels on our [Slack](

## Developing

*Node*: Check that Node is [installed](https://nodejs.org/en/download/) with version 10.19.0 and up. You can check this with `node -v`.
*Node*: Check that Node is [installed](https://nodejs.org/en/download/) with version `^12.16 || >= 14`. You can check this with `node -v`.

*Yarn*: Make sure that Yarn 1 is [installed](https://classic.yarnpkg.com/en/docs/install) with version >= `1.19.0`.

Expand Down

9 comments on commit 87f264c

@jedwards1211
Copy link
Contributor

@jedwards1211 jedwards1211 commented on 87f264c Jul 20, 2021 •
edited

Choose a reason for hiding this comment

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

@JLHwung I failed to bootstrap on Node 12.16.3 and 14.11.0...mjs changes have broken things.
Bootstrapping succeeded with 12.22.1 and 14.17.3. I don't know exactly what the correct fix would be

On 12.16.3:

/Applications/Xcode.app/Contents/Developer/usr/bin/make generate-tsconfig build
yarn node scripts/generators/tsconfig.js
internal/modules/cjs/loader.js:1149
      throw new ERR_REQUIRE_ESM(filename, parentPath, packageJsonPath);
      ^

Error [ERR_REQUIRE_ESM]: Must use import to load ES Module: /Users/andy/github/babel/scripts/generators/tsconfig.js
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1149:13)
    at Module.load (internal/modules/cjs/loader.js:977:32)
    at Function.Module._load (internal/modules/cjs/loader.js:877:14)
    at Function.executeUserEntryPoint [as runMain] (internal/modules/run_main.js:74:12)
    at internal/main/run_main_module.js:18:47 {
  code: 'ERR_REQUIRE_ESM'
}
make[1]: *** [generate-tsconfig] Error 1
make: *** [bootstrap] Error 2

On 14.11.0:

yarn gulp build
file:///Users/andy/github/babel/gulpfile.mjs:21
import { Worker as JestWorker } from "jest-worker";
         ^^^^^^
SyntaxError: The requested module 'jest-worker' is expected to be of type CommonJS, which does not support named exports. CommonJS modules can be imported by importing the default export.
For example:
import pkg from 'jest-worker';
const { Worker: JestWorker } = pkg;
    at ModuleJob._instantiate (internal/modules/esm/module_job.js:98:21)
    at async ModuleJob.run (internal/modules/esm/module_job.js:137:5)
    at async Loader.import (internal/modules/esm/loader.js:165:24)
make: *** [build-bundle] Error 1

@hzoo
Copy link
Member

@hzoo hzoo commented on 87f264c Jul 20, 2021

Choose a reason for hiding this comment

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

I guess we could always update this note at the very least to those versions? Maybe make a pr for it

@jedwards1211
Copy link
Contributor

Choose a reason for hiding this comment

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

@hzoo okay I found this comment: nodejs/node#32137 (comment)

Node.js 14.13.0 added support for named CJS exports, see #35249 and this change has also been backported to Node.js v12.20.0.

I'll test on those versions...

@JLHwung
Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch. On CI we build Babel on latest node version (now v16). Chances are it might break in older versions and we forget to update build requirements. Maybe we should just specify building requires latest node current?

@jedwards1211
Copy link
Contributor

@jedwards1211 jedwards1211 commented on 87f264c Jul 20, 2021 •
edited

Choose a reason for hiding this comment

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

Maybe just pick versions and add CI tasks to make sure bootstrapping works on those versions of node? That way the version isn't constantly up in the air and we get early warning if something breaks dev compatibility

It would be good to make the bootstrap script check the node version and error out if it's less than the targeted compatible versions, to reduce confusion for first-time contributors. (With a flag to bypass that if we want to debug whether it works on lower versions). I could PR that if you're interested

@JLHwung
Copy link
Contributor

Choose a reason for hiding this comment

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

pick versions and add CI tasks to make sure bootstrapping works on those versions of node

We use latest node on CI for performance and freedom to refactor build scripts. Babel used to build in every supported node versions, but later we extracted the build step to improve CI running time. The build scripts will be constantly run by contributors. As long as we are happy to try out latest node versions, I don't think we need an extra CI step.

make the bootstrap script check the node version and error out

That sounds great! We used to specify the minimal building node versions by engines.node of the root package.json, which is supported in Yarn 1. However, Yarn 2 does not support the engine check. If we are going to bring it back, the node version can be put there.

@merceyz I am not sure if engines.node check is in the scope of yarn constraints, WDYT?

@jedwards1211
Copy link
Contributor

@jedwards1211 jedwards1211 commented on 87f264c Jul 21, 2021 •
edited

Choose a reason for hiding this comment

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

@JLHwung

Bootstrapping takes a minute or so, why not just add that to the Node 12 and 14 test tasks? Investigating this has cost me at least 30 minutes.
Is 30 minutes confusion for many new contributors worth a change as mundane as converting a bunch of requires to imports?
It seems reckless to me if anyone can use new node features without warning in build scripts, without anyone being aware of the history of those new features, as happened in this case. I think it saves time and disruption to only refactor once in awhile.
Also checking if the user has the latest version of node installed before each build script would fail if a dev is away from internet. To avoid that you have to pick a fixed version of node.

@jedwards1211
Copy link
Contributor

@jedwards1211 jedwards1211 commented on 87f264c Jul 21, 2021 •
edited

Choose a reason for hiding this comment

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

Maybe I misunderstand though, maybe by "latest node" you just mean >=16? I was thinking you mean literally the absolute latest version of node on any given day

@merceyz
Copy link
Contributor

@merceyz merceyz commented on 87f264c Aug 1, 2021

Choose a reason for hiding this comment

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

I am not sure if engines.node check is in the scope of yarn constraints, WDYT?

Constraints is more for enforcing the project itself not the environment it runs under 馃

Please sign in to comment.