[babel 8] Align `@babel/node` args parsing to Node.js by nicolo-ribaudo · Pull Request #16706 · babel/babel

nicolo-ribaudo · 2024-07-31T16:14:48Z

Q	A
Fixed Issues?	Fixes #16704, fixes #4373, fixes #13730
Patch: Bug Fix?
Major: Breaking Change?
Minor: New Feature?
Tests Added + Pass?	Yes
Documentation PR Link	babel/website#2950
Any Dependency Changes?
License	MIT

This aligns args parsing of @babel/node to how Node.js parses arguments:

there is an optional inspect sub-command
then, there are all args for Node.js, V8, and Babel, in any order
optionally, there is a --
if not using the -e or -p flags, there is either - or a filepath
then, there are all the arguments to be passed to the script

This means that whether an argument is passed to process.argv or not does not depend on its name/value, but simply on its position.

To properly parse arguments, we need a list of arguments that take a value, so that we can know that in babel-node --require foo bar bar is the script, while in babel-node --expose-gc foo bar foo is the script. This is auto-generated, our CI will check that it includes all the flags from all the Node.js versions, so that it fails as soon as a new Node.js version is released telling us to add the flag if needed.

nicolo-ribaudo · 2024-07-31T16:15:29Z


-const require = createRequire(import.meta.url);
-
-const program = process.env.BABEL_8_BREAKING


All the removed lines from this file are just moved to program-setup.ts

nicolo-ribaudo · 2024-07-31T16:15:56Z

+    process.exitCode = 1;
+  });
+} else {
+  const args = [babelNodePath];


All this else block is indented -- review with hidden whitespace diff

nicolo-ribaudo · 2024-07-31T16:16:48Z

@@ -0,0 +1,4 @@
+{
+  "BABEL_8_BREAKING": false,
+  "args": ["foo", "-r", "./dep"]


This exmaple shows different Babel 7 vs Node.js behavior

babel-bot · 2024-07-31T16:17:05Z

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/57863

liuxingbaoyu · 2024-07-31T16:55:59Z

Amazing!
Given that https://www.npmjs.com/package/kexec only gets a few hundred downloads per week, perhaps we can remove its use in Babel 8.

#11402

liuxingbaoyu · 2024-07-31T17:56:39Z

Also fixes #4373 and #13730 !

nicolo-ribaudo · 2024-08-01T14:44:44Z

Removing kexec sounds good, I'll open a follow up PR for it.

liuxingbaoyu · 2024-08-02T19:14:29Z

The problem seems to come from here, but I'm not sure how to fix it elegantly. :)

var _nodeFlagsWithValue2 = require("../data/node-flags-with-value.json");
const nodeFlagsWithValue = new Set(_nodeFlagsWithValue2.default);

nicolo-ribaudo · 2024-08-04T21:03:03Z

Thanks for investigating! I pushed a change that hopefully fixes this.

JLHwung · 2024-08-05T13:24:47Z

@@ -1,4 +1,4 @@
 {
-  "args": ["foo", "--extensions", ".bar"],
+  "args": ["--extensions", ".bar", "foo"],


If we exit the splitArgs program args scan without seeing --, can we scan the userArgs and make sure there is no match in node flags? If we have seen such match, we can print a warning message such as:

--extensions seems to be a babel-node/node option. Did you mean babel-node --extensions .bar foo? If you intend to pass --extensions to your script, please add -- after foo: babel-node foo -- --extensions. bar

Of course please improve the wordings here. This message should help people during their upgrades from 7 to 8.

If there are multiple matches, we can group the messages and provide one final suggestion.

Will this give rise to false positives? If so I would prefer to keep it as is.

This will indeed give false positive: for example, if you have a script that accepts an --extensions parameter, it will warn. However, it is easy to suppress the warning (by simply adding --), so it could be ok?

We could also remove the warning, for example, after that Babel 8 downloads surpass Babel 7 downloads, so that it's only there for the migration period.

That sounds good. :)

JLHwung · 2024-08-23T13:26:38Z

    "--expose-gc-as=gc2",
    "--config-file",
    "./config.json",
+    "--extensions=.js,.ts",


Do we print warning if there are only node options after the script name? E.g.

babel-node ./foo.js --expose_gc

We do not, because it would require us to keep the v8flags dependency just for that warning.

However, I'm less worried about that case since when using the Node.js CLI you already have to pass Node-specific options before the script name.

I think it is fine to keep or inline the v8flags dependency and finally remove it in Babel 9. In Babel 7 the node options after the script name are still passed to node and we align the args parsing only starting from this Babel 8 PR.

For example, I searched babel-node [script name] --max-old-space-size=, while the majority get the ordering right, there are indeed a few usage where node options are after the script name, because it simply works for Babel 7:

https://github.com/Ocelyn/r3f-next-starter/blob/eaa6ad70409943ca6a6cb05e6e502ae994847a0d/package.json#L12

https://github.com/chairemobilite/evolution/blob/0aecdc436411fa15ef9991a3309c8e2898c2700c/example/demo_survey/package.json#L21

https://github.com/adieuadieu/japan-train-data/blob/0a31ec2665475c3e46da7ffc7786c33be9e7ac05/package.json#L38

Imagine if they upgrade to Babel 8, their app may stop working due to insufficient memory size. However, it will be a really confusing error: they might keep increasing the --max-old-space-size until they figure out that --max-old-space-size after the script name are no longer passed to the node. We could save them lots of time if we just keep v8flags for a while and remind them to update their usage.

We can also start printing warnings in Babel 7 if node options after the script name is seen, since they will not be supported in Babel 8.

JLHwung · 2024-08-26T14:37:53Z

+        `Run node ./packages/babel-node/scripts/list-node-flags.js --update to update it.`
+    );
+    // eslint-disable-next-line n/no-process-exit
+    process.exit(1);


Nit: we can just throw Missing flag as an error so that the process will exit with non-zero code.

nicolo-ribaudo · 2024-09-04T14:39:30Z

@JLHwung I added the warning for Node.js / V8 options.

JLHwung · 2024-09-05T15:56:22Z

+            `passing the -- separator before the script name:\n` +
+            `    babel-node ${programArgs.join(" ")} -- ${fileName} ${userArgs.join(" ")}\n` +
+            `  If the intention is to pass ${them} to Babel, move ${them} before the filename:\n` +
+            `    babel-node ${programArgs.join(" ")} ${ambiguousArgs.join(" ")} ${fileName} ${unambiguousArgs.join(" ")}\n`,


Nit: Can we swap these two? I assume most people are intended to pass options to Babel / Node so let's offer the majority choice first.

liuxingbaoyu

Amazing!

nicolo-ribaudo added pkg: node PR: Bug Fix (next major) 🐛 A type of pull request used for our changelog categories for next major release labels Jul 31, 2024

nicolo-ribaudo commented Jul 31, 2024

View reviewed changes

nicolo-ribaudo force-pushed the babel-8-node-args branch from 4a907a7 to b85f359 Compare July 31, 2024 16:18

nicolo-ribaudo added the PR: Needs Docs label Jul 31, 2024

nicolo-ribaudo force-pushed the babel-8-node-args branch from 8524fe3 to 9c92f3e Compare August 2, 2024 13:43

liuxingbaoyu force-pushed the babel-8-node-args branch 3 times, most recently from 6080ef1 to 38e9fb9 Compare August 2, 2024 18:26

nicolo-ribaudo mentioned this pull request Aug 4, 2024

Update babel-plugin-polyfill-corejs3 #16715

Merged

nicolo-ribaudo force-pushed the babel-8-node-args branch from 4ce39e5 to cd0ca4f Compare August 5, 2024 13:13

JLHwung reviewed Aug 5, 2024

View reviewed changes

nicolo-ribaudo force-pushed the babel-8-node-args branch from cd0ca4f to 8d92925 Compare August 23, 2024 12:54

nicolo-ribaudo commented Aug 23, 2024

View reviewed changes

Comment thread packages/babel-node/scripts/list-node-flags.js Outdated

JLHwung reviewed Aug 23, 2024

View reviewed changes

JLHwung reviewed Aug 26, 2024

View reviewed changes

nicolo-ribaudo added 6 commits September 3, 2024 22:14

Align @babel/node args parsing to Node.js

b7435d0

Add missing Node.js 12 and 10 flags

99715fe

Fix import compilation

ade81fc

Add warning

d6b471a

Update script

e5c8ddc

Add warning for Node.js/V8 flags

5416d16

nicolo-ribaudo force-pushed the babel-8-node-args branch from 1fef072 to 5416d16 Compare September 4, 2024 14:39

Update stderr

9525a09

JLHwung reviewed Sep 5, 2024

View reviewed changes

JLHwung approved these changes Sep 5, 2024

View reviewed changes

liuxingbaoyu approved these changes Sep 5, 2024

View reviewed changes

Reorder error message

8404087

nicolo-ribaudo mentioned this pull request Sep 6, 2024

[babel 8] Docs for babel/node args order babel/website#2950

Merged

nicolo-ribaudo removed the PR: Needs Docs label Sep 6, 2024

nicolo-ribaudo merged commit af1662b into babel:main Sep 6, 2024

nicolo-ribaudo deleted the babel-8-node-args branch September 6, 2024 11:06

github-actions Bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Dec 7, 2024

github-actions Bot locked as resolved and limited conversation to collaborators Dec 7, 2024


		const require = createRequire(import.meta.url);

		const program = process.env.BABEL_8_BREAKING

Uh oh!

Uh oh!

Conversation

nicolo-ribaudo commented Jul 31, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

babel-bot commented Jul 31, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

liuxingbaoyu commented Jul 31, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

liuxingbaoyu commented Jul 31, 2024

Uh oh!

nicolo-ribaudo commented Aug 1, 2024

Uh oh!

liuxingbaoyu commented Aug 2, 2024

Uh oh!

nicolo-ribaudo commented Aug 4, 2024

Uh oh!

JLHwung Aug 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JLHwung Aug 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicolo-ribaudo commented Sep 4, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liuxingbaoyu left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

nicolo-ribaudo commented Jul 31, 2024 •

edited

Loading

babel-bot commented Jul 31, 2024 •

edited

Loading

liuxingbaoyu commented Jul 31, 2024 •

edited

Loading

JLHwung Aug 5, 2024 •

edited

Loading

JLHwung Aug 26, 2024 •

edited

Loading