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

Detect non-v8 flags #55

Merged
merged 1 commit into from
May 31, 2020
Merged

Detect non-v8 flags #55

merged 1 commit into from
May 31, 2020

Conversation

snoack
Copy link
Contributor

@snoack snoack commented May 30, 2020

In particular newer versions of Node.js have a bunch of non-v8 flags that are relevant in particular when using v8flags in combination with flagged-respawn or liftoff (like Gulp does), e.g. --experimental-modules.

@phated
Copy link
Member

phated commented May 31, 2020

This is an awesome change! Thank you!

I ran node --help and looked through some of their flags and I wonder if we should exclude some others like --exec, --print, --interactive (and their short flags) and any that conflict with gulp flags, like --require and --version. What do you think @snoack ?

@snoack
Copy link
Contributor Author

snoack commented May 31, 2020

--exec, --print, --interactive, --require and --version were implicitly ignored as they are documented like -v, --version which doesn't match our regular expression /\s\s--[\w-]+/gm.

Anyway, I just discovered that there is process.allowedNodeEnvironmentFlags for discovering non-v8 flags (excluding the above except for --require), and started using it here instead.

As for --require, it seems to be a legit flag that can be useful at times. FWIW Gulp's --require option seems redundant with the built-in option as it seems to do exactly the same.

What do you think?

@phated
Copy link
Member

phated commented May 31, 2020

I looked into that in the past and I thought there was an issue with it. Let me try to find that previous work.

@phated
Copy link
Member

phated commented May 31, 2020

Ah, my previous work was at #51 and I was hoping to remove this module.

I think combining both might do the trick, but I'd like to support older nodes on the first version release of this. Can you restore the node --help support in this PR and then we will land the other changes when we drop node<10 support in the next major?

@snoack
Copy link
Contributor Author

snoack commented May 31, 2020

Are you talking about #51? It seems the issue there was that process.allowedNodeEnvironmentFlags doesn't include v8 flags, hence it cannot replace calling node --v8-options. But here I augment the result of node --v8-options with the contents of process.allowedNodeEnvironmentFlags in order to get both v8 flags and Node.js specific flags.

@phated
Copy link
Member

phated commented May 31, 2020

@snoack yep! posted that right before you 😛

What do you think of my "phased" suggestion to getting this to eventually use that API?

@snoack
Copy link
Contributor Author

snoack commented May 31, 2020

You beat me to it. :)

Can you restore the node --help support in this PR [...]?

Done.

What do you think of my "phased" suggestion to getting this to eventually use that API?

Personally, I think it would be fine start using process.allowedNodeEnvironmentFlags if available. The way I implemented it, on Node.js <10.10 you simply would get v8 flags only. But your call.

@phated phated merged commit c77b2c8 into gulpjs:master May 31, 2020
@snoack
Copy link
Contributor Author

snoack commented May 31, 2020

Thanks for merging! Want to release a new version? Then I can update the dependency in gulpjs/gulp-cli#214 rather than adding a workaround.

@phated
Copy link
Member

phated commented May 31, 2020

Yep, I'm just finishing up a couple things and then letting CI run.

@phated
Copy link
Member

phated commented May 31, 2020

v3.2.0 published. Thanks for working on this!

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.

2 participants