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

deprecate --use_strict #6429

Closed
ljharb opened this issue Apr 27, 2016 · 29 comments
Closed

deprecate --use_strict #6429

ljharb opened this issue Apr 27, 2016 · 29 comments
Labels
v8 engine Issues and PRs related to the V8 dependency.

Comments

@ljharb
Copy link
Member

ljharb commented Apr 27, 2016

I'd like to reopen the discussion from nodejs/node-v0.x-archive#7479 if possible about somehow deprecating/removing this flag.

Whether the v8 team is willing to remove or rename it, or not (which we should probably re-investigate), I would like node to find a way to have it be removed or renamed.

@Fishrock123
Copy link
Contributor

cc @nodejs/v8

@Fishrock123 Fishrock123 added the v8 engine Issues and PRs related to the V8 dependency. label Apr 27, 2016
@Fishrock123
Copy link
Contributor

Arguments are passed verbatim to v8, I'm not sure we're keen on altering that..

@mscdex
Copy link
Contributor

mscdex commented Apr 27, 2016

I'm -1 on interfering with (or documenting) command-line options provided by v8.

@vkurchatkin
Copy link
Contributor

-1, this flag is useful

@Fishrock123
Copy link
Contributor

-1, this flag is useful

@vkurchatkin Mind to elaborate on that?

@ljharb
Copy link
Member Author

ljharb commented Apr 27, 2016

How is it useful? The flag is not the same as "use strict", and it's dangerous to execute sloppy code in strict mode anyways.

@cjihrig
Copy link
Contributor

cjihrig commented Apr 27, 2016

As recently as #6377, we rejected documenting V8 flags. This goes even further beyond that. I think it would be simplest for people to just not use it.

@ljharb
Copy link
Member Author

ljharb commented Apr 27, 2016

The reasons for rejecting documentation is given as "… those are not under our control to change but as soon as we document it they become our responsibility to maintain". This is different - this is ensuring with certainty that no node users could ever depend on the v8 flag, which reduces the maintenance needed from the node core team (aside from maintaining a blacklist, for example)

@ljharb
Copy link
Member Author

ljharb commented Apr 27, 2016

In other words, if --use_strict was removed by the v8 team, it's a breaking change for node anyways, documented or not. Blacklisting it lets you control when the breaking change occurs, and prevents it from ever occurring in the future.

@vkurchatkin
Copy link
Contributor

Don't want to state the obvious, but it's useful when you want to run all javascript in strict mode. I think it's well understood that strict mode is not the default mode only because it's not backwards compatible.

@ljharb
Copy link
Member Author

ljharb commented Apr 27, 2016

@vkurchatkin yes but --use_strict is not JavaScript, and at least in past versions of node, even some internal modules failed with this flag. If you want to run all of your JS in strict mode, you'd use ES6 modules (babel-transpiled, or one day, native) or put "use strict" on the top of every file - when would you want to run code you didn't author in a nonstandard mode?

@vkurchatkin
Copy link
Contributor

I agree that most people don't need it, but that doesn't mean that we should remove it.

For example I use v8 on android to run some js in strict mode and use node to test this code on desktop. There are no dependencies, so I use --use_strict

@trevnorris
Copy link
Contributor

@ljharb There are 389 options in node --v8-options, and I'm pretty sure ones like --use_strong, or --strong_this or --fast_math or --expose_wasm or --expose_gc or --allow_natives_syntax (all of these I do use in development), are "not JavaScript". This discussion revolves around an obscure flag, just like many others, that sit outside our direct control. It seems strange we'd consider removing only one of potentially many "not JavaScript" or not "useful" ones without taking into consideration all the other flags that should also be removed.

Also, what evidence is there that this flag is actually problematic or harmful in the ecosystem?

@ChALkeR
Copy link
Member

ChALkeR commented Apr 28, 2016

@trevnorris People sometimes report issues like «your module is broken and doesn't work … I used --use_strict» to various repos.

Still, I don't think that blacklisting this would do any good — some people might need it, as @vkurchatkin noted, and there are a lot more flags that affect the behaviour in a unsafe way, and not only the ones that @trevnorris noted. E.g. --harmony-* flags often make the process crash in certain situations, etc.

IMO, we shouldn't blacklist anything, but we could print a warning if certain flags are detected (without modifying the flags, though). But such warning should cover much more than just --use-strict — if we introduce such warnings, it would make sense to warn against --harmony flags (reminding that those should be used only for testing, not in production).

Btw, there is even such a thing as https://github.com/isaacs/use-strict.

@Fishrock123
Copy link
Contributor

I'd be in favor of warning against --use_strict and --harmony and --es_staging.

@ChALkeR
Copy link
Member

ChALkeR commented Apr 28, 2016

@Fishrock123 And --harmony-*. 😉

@ljharb
Copy link
Member Author

ljharb commented Apr 28, 2016

Note as well that if node truly wants VM neutrality, blindly passing through engine flags isn't going to be sustainable - so node may soon have to maintain an exclusion list/inclusion list of flags anyways.

@jasnell
Copy link
Member

jasnell commented Apr 28, 2016

@Fishrock123 ... docs warning or runtime warning?

@trevnorris
Copy link
Contributor

@ljharb I don't see maintaining that type of exclusion list as an option. It would be impossible to debug and tune for individual VMs if all options to do so were removed. For example, while the v8 module name is specific to the VM, it should be left there for v8 builds. It provides important information if ever debugging a VM specific issue. Similar to what we've been troubleshooting with GC and Typed Arrays.

@Fishrock123
Copy link
Contributor

Note as well that if node truly wants VM neutrality, blindly passing through engine flags isn't going to be sustainable - so node may soon have to maintain an exclusion list/inclusion list of flags anyways.

Could you elaborate why it wouldn't be? I don't see any problem here. (Also, developing our own vm flags is pretty silly and out of scope for sure.)

@Fishrock123
Copy link
Contributor

@jasnell presumably runtime, might have to be in c++ though I think, since this happens before the VM boots up.

@vkurchatkin
Copy link
Contributor

@ljharb this is one of the reasons why true VM neutrality is very unlikely. Each VM has it's own flags for tuning critical subsystems (e.g. GC), which will be very hard to reconcile

@Fishrock123
Copy link
Contributor

I don't see a reason we have to be VM neutral on flags when we can just keep passing flags to the VM that is being used.

@jasnell
Copy link
Member

jasnell commented Apr 28, 2016

@Fishrock123 ... yep, I've had it on my todo to see if we can hook into process warning easily from native but a simple message printed to stderr on startup works for me.... Perhaps it would be worthwhile printing the warning when any VM specific flag is used? If --no-warnings flag is used, go ahead and skip the warning tho.

@ChALkeR
Copy link
Member

ChALkeR commented Apr 28, 2016

@jasnell I don't think this justifies warnings on debug flags, e.g. --trace-gc.

@bnoordhuis
Copy link
Member

bnoordhuis commented Apr 28, 2016

FTR, I disagree completely with blacklisting or warning. I'm an adult, I don't need a computer program telling me what I should or shouldn't do. It's paternalistic.

@ChALkeR
Copy link
Member

ChALkeR commented Apr 28, 2016

@bnoordhuis, this isn't something like "don't do this" kind of warning, I was thinking of something that is more like a friendly reminder, e.g. "Notice: using flagged harmony features might not work as expected and might even crash the process".

@littledan
Copy link

About forwarding flags: Chromium embeds V8 by forwarding flags through a special option, --js-flags=<things-to-forward-to-v8>. There are then some Chromium flags, like --javascript-harmony, which explicitly forwards into --harmony to V8. This strategy seems to work well for us. The V8 team frequently adds and removes flags, especially through the lifecycle of developing a new feature, staging it, shipping it, and then removing the conditionality of the feature as it is cemented into the code, so flags are not guaranteed to be a stable interface.

saghul added a commit to saghul/sjs that referenced this issue Jun 13, 2016
Users should decide if they want strict mode or not. I initially added
it because Node had it, but it turns out it's a V8 flag really, not a
Node flag, and it's not really recommended to use.

Refs: nodejs/node#6429
saghul added a commit to saghul/sjs that referenced this issue Jun 13, 2016
Users should decide if they want strict mode or not. I initially added
it because Node had it, but it turns out it's a V8 flag really, not a
Node flag, and it's not really recommended to use.

Refs: nodejs/node#6429
@bnoordhuis
Copy link
Member

bnoordhuis commented Jun 27, 2016

I'm closing this issue, it's been idle for some time, but reopen if you disagree. I believe the dominant sentiment is that we won't be adopting this proposal.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

No branches or pull requests

10 participants