vm: move options checks from C++ to JS #19398

targos · 2018-03-16T18:31:13Z

~~I tried to keep the type checking as it was done in C++ (not very strict for some options).~~ -> Now all options are checked more strictly.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

devsnek · 2018-03-16T19:06:56Z

lib/vm.js

@@ -206,13 +258,20 @@ function runInNewContext(code, sandbox, options) {
  options = Object.assign({}, options, {
    [kParsingContext]: sandbox
  });
-  return createScript(code, options).runInNewContext(sandbox, options);
+  return createScript(code, options).runInContext(sandbox, options);


this should still be runInNewContext right?

Right, thanks.

bnoordhuis · 2018-03-16T18:56:44Z

src/module_wrap.cc

+
+MaybeLocal<Integer> GetLineOffsetArg(Environment* env,
+                                     Local<Value> options) {
+  Local<Integer> defaultLineOffset = Integer::New(env->isolate(), 0);


Style: default_line_offset

I just moved those functions from node_contextify.cc without modifying them. I'd prefer to leave them like that because the goal is to completely remove them in follow-up PR.

I reverted that move. It isn't necessary.

bnoordhuis · 2018-03-16T18:57:03Z

src/module_wrap.cc

+
+MaybeLocal<Integer> GetColumnOffsetArg(Environment* env,
+                                       Local<Value> options) {
+  Local<Integer> defaultColumnOffset = Integer::New(env->isolate(), 0);


bnoordhuis · 2018-03-16T18:58:35Z

src/module_wrap.cc

+    return defaultColumnOffset;
+
+  return value->ToInteger(env->context());
+}


These two functions have a lot of common code that can be factored out.

bnoordhuis · 2018-03-16T18:59:10Z

src/node_contextify.cc

+  Utf8Value name_val(env->isolate(), options.name);
+  ctx->AllowCodeGenerationFromStrings(options.allowCodeGenStrings);
+  ctx->SetEmbedderData(ContextEmbedderIndex::kAllowWasmCodeGeneration,
+      options.allowCodeGenWasm);


Can you line up the argument?

bnoordhuis · 2018-03-16T19:00:30Z

src/node_contextify.cc

+    Local<String> filename = args[1].As<String>();
+
+    Local<Integer> lineOffset;
+    Local<Integer> columnOffset;


Snake case.

bnoordhuis · 2018-03-16T19:06:26Z

src/node_contextify.cc

-    bool display_errors = maybe_display_errors.ToChecked();
-    bool break_on_sigint = maybe_break_on_sigint.ToChecked();
+    CHECK(args[0]->IsNumber());
+    int64_t timeout = args[0].As<Integer>()->Value();


The check doesn't match the cast. It's not exactly wrong because Integer::Value() does the right thing regardless but stil..

You could instead use IntegerValue() (but please use the overload that takes a Local<Context>.)

bnoordhuis · 2018-03-16T19:10:00Z

src/node_contextify.cc

+    int64_t timeout = args[0].As<Integer>()->Value();
+
+    CHECK(args[1]->IsBoolean());
+    bool display_errors = args[1].As<Boolean>()->Value();


Maybe just args[1]->IsTrue()?

bnoordhuis · 2018-03-16T19:10:42Z

src/node_contextify.cc

+    bool display_errors = args[2].As<Boolean>()->Value();
+
+    CHECK(args[3]->IsBoolean());
+    bool break_on_sigint = args[3].As<Boolean>()->Value();


Last two comments apply here as well.

bnoordhuis · 2018-03-16T19:11:04Z

src/node_contextify.h

+  v8::Local<v8::String> name;
+  v8::Local<v8::String> origin;
+  bool allowCodeGenStrings;
+  v8::Local<v8::Boolean> allowCodeGenWasm;


snake_case

This mixing of bool and v8::Boolean is kind of weird although I understand why it is that way.

TimothyGu · 2018-03-17T03:10:44Z

Partial dup of #19268

targos · 2018-03-17T10:40:08Z

@TimothyGu thanks for pointing it out. Let's focus on landing #19268 and I'll update this.

targos · 2018-03-17T12:26:03Z

Interesting side-effect: there is a non-negligible perf improvement:

                                                                        confidence improvement accuracy (*)   (**)  (***)
 vm/run-in-context.js withSigintListener=0 breakOnSigint=0 n=10000             ***     24.31 %       ±3.80% ±5.11% ±6.76%
 vm/run-in-context.js withSigintListener=0 breakOnSigint=1 n=10000             ***      3.95 %       ±2.12% ±2.83% ±3.69%
 vm/run-in-context.js withSigintListener=1 breakOnSigint=0 n=10000             ***     22.03 %       ±4.32% ±5.79% ±7.61%
 vm/run-in-context.js withSigintListener=1 breakOnSigint=1 n=10000              **      3.38 %       ±2.43% ±3.24% ±4.22%
 vm/run-in-this-context.js withSigintListener=0 breakOnSigint=0 n=10000        ***     58.57 %       ±3.55% ±4.72% ±6.15%
 vm/run-in-this-context.js withSigintListener=0 breakOnSigint=1 n=10000        ***     11.36 %       ±2.59% ±3.45% ±4.50%
 vm/run-in-this-context.js withSigintListener=1 breakOnSigint=0 n=10000        ***     59.95 %       ±4.09% ±5.44% ±7.09%
 vm/run-in-this-context.js withSigintListener=1 breakOnSigint=1 n=10000        ***     11.57 %       ±2.10% ±2.79% ±3.63%

targos · 2018-03-22T15:50:32Z

lib/vm.js

+      lineOffset = 0,
+      columnOffset = 0,
+      cachedData,
+      produceCachedData = false,


I'd like to take the opportunity to do stricter verifications of the *Offset and produceCachedData options. Good idea?

targos · 2018-03-22T15:51:57Z

lib/vm.js

+  }
+  const {
+    displayErrors = true,
+    breakOnSigint = false


ditto: I'd like to validate the type of all options

targos · 2018-03-22T15:53:12Z

Rebased. I still have a question about options validation.

joyeecheung

Good work! Left some comments

joyeecheung · 2018-03-22T23:33:21Z

lib/vm.js

+  } else {
+    timeout = Number(timeout);
+    if (timeout <= 0) {
+      throw new ERR_OUT_OF_RANGE('options.timeout', 'a positive number');


Can you pass the timeout to the constructor so it gets displayed in the error message?

joyeecheung · 2018-03-22T23:33:45Z

lib/vm.js

+    throw new ERR_INVALID_ARG_TYPE('contextifiedSandbox', 'Object', sandbox);
+  }
+  if (!_isContext(sandbox)) {
+    throw new ERR_INVALID_ARG_TYPE('contextifiedSandbox', 'vm.Context');


Can you pass sandbox to the constructor?

Because at this point, sandbox is guaranteed to be an object, it seems confusing to me if we print "Received type object" in this case.

@targos Then perhaps this should be ERR_INVALID_ARG_VALUE?

joyeecheung · 2018-03-22T23:34:48Z

lib/internal/bootstrap/loaders.js

-    const script = new ContextifyScript(code, options);
-    return script.runInThisContext();
+  function runInThisContext(code, filename) {
+    const script = new ContextifyScript(code, filename);


At this point do we even need this helper? Maybe just inline it in the try-catch block?

targos · 2018-03-23T11:25:00Z

Updated again. PTAL

targos · 2018-03-29T05:25:16Z

Ping :)

CI: https://ci.nodejs.org/job/node-test-pull-request/13918/

targos · 2018-03-29T10:55:39Z

CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1345/

targos · 2018-03-29T11:23:37Z

CITGM found some real bugs 🎉 !

Fixing...

targos · 2018-03-30T11:00:06Z

So... This breaks gulp 3 because it depends on

vinyl-fs, which depends on
graceful-fs, which depends on
natives that uses process.binding('contextify').ContextifyScript directly.

How do we proceed?

/cc @nodejs/tsc

mcollina · 2018-03-30T11:12:56Z

Gulp 3 depends on a very old version of vynil-fs (0.3.0) https://github.com/gulpjs/gulp/blob/v3.9.1/package.json#L41. vynil-fs since version 2.0.0 uses gracefuls-fs@4, which does not depend on natives anymore. IMHO we should see if it's possible to update gulp@3 to vynil-fs@2.
Alternatively, we have to update vynil-fs@0.3.x to graceful-fs@4.

@phated what do you think?

bnoordhuis · 2018-03-30T11:20:44Z

Yeah, that was already a known issue two years ago, see #5213 and #8149 (comment) from @phated.

I think we made peace with the fact that projects that don't upgrade stop working sooner or later. We probably don't want to back-port but for sure it can go into Node.js 10.

gulp 4 is not affected, by the way, see gulpjs/gulp#1604.

addaleax · 2018-03-30T11:24:44Z

I agree with Ben, but just in case it matters: I have taken over maintenance for natives, and for smaller things like this, I can probably fix the module up in some way. But the hacks to keep it alive are getting worse and worse over time, and I’d estimate that within the next 6 months or so, maintaining it will be not possible anymore (which is a good thing imo).

mcollina · 2018-03-30T11:25:09Z

Unfortunately npm install gulp will install gulp@3, not gulp@4. Here is the output of npm show gulp:

{ name: 'gulp',
  description: 'The streaming build system',
  'dist-tags': { latest: '3.9.1', next: '4.0.0' },

targos · 2018-03-30T13:07:19Z

New CI: https://ci.nodejs.org/job/node-test-pull-request/13944/
CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1346/

mcollina · 2018-04-05T15:25:02Z

@jdalton gulp@3 depends on vinyl-fs@0.3.0, which depends on graceful-fs@3.0.0. The approach used in graceful-fs@3.0.0 is deprecated and unmaintainable. If gulp@3 wants to keep working on Node 10, it needs to update vinyl-fs to graceful-fs@4.0.0 (I'm not sure what this would entail).

gulp@4 is unaffected.

jdalton · 2018-04-05T15:30:34Z

gulp@3 depends on vinyl-fs@0.3.0, which depends on graceful-fs@3.0.0. The approach used in graceful-fs@3.0.0 is deprecated and unmaintainable.

So natives needs to be tweaked?

If gulp@3 wants to keep working on Node 10, it needs to update vinyl-fs to graceful-fs@4.0.0 (I'm not sure what this would entail).

So natives doesn't need to be tweaked?

It would be great, if Node needs action from folks maintaining projects, for you all to clearly spell out what is requested from each project (pointing to line numbers, versions, and possible solutions). This might be done by opening issues on those projects, to give project specific context, and linking the issues back here or #19786 for broader discussion.

yocontra · 2018-04-05T15:35:52Z

@mcollina Yeah, that's the interesting thing. We would have to push a patch release to vinyl-fs 0.3 that relies on graceful-fs v4, but IIRC there were more changes in graceful-fs v3 -> v4 that were breaking changes that would bubble up to vinyl-fs and thus gulp. I'd rather not commit semver violations (and break our users code) if we don't absolutely have to.

If there was a clean patch release made to graceful-fs v3 then we don't even have to do anything - it will get automatically pulled in by vinyl-fs 0.3. I'm not sure why we're being pulled into this when the breakage isn't in our project, but in one of our dependencies. If any project should be backporting fixes it should be graceful-fs (not that this is their fault - see #8149 (comment)).

Correct me if I'm missing information here as I've just hopped into this, I know @phated has been dealing with this for a long time (#8149 (comment) 2016) and I feel like our response from gulp has been pretty consistent.

mcollina · 2018-04-05T16:14:47Z

@contra as far as I understand, the breaking changes in graceful-fs@4 are needed because of the removal of natives. So, having graceful-fs@3 compatible with Node 10 is not possible. (@jdalton I hope this answers your question).

Correct me if I'm missing information here as I've just hopped into this, I know @phated has been dealing with this for a long time (#8149 (comment) 2016) and I feel like our response from gulp has been pretty consistent. Talk to @isaacs about backporting a graceful-fs v3 fix.

As far as I get it, you have been pinged by @jdalton because we are at an impasse. The net result would be that gulp@3 would likely not work on Node 10, or maybe 11 (if @addaleax can manage to get us those extra 6 months).

On the good side, it might be a good moment to push for upgrading to gulp@4.

jasnell · 2018-04-05T16:19:27Z

Fundamentally this is a problem in natives and in graceful-fs. That's where the effort should be put. That, of course, will have a downstream effect, but that cannot be avoided.

MylesBorins · 2018-04-05T16:26:12Z

FWIW I had originally blocked on the PR and only removed my -1 based on the commitment that Node 10 would be shipped in a way that would not break Gulp 3.

If we cannot find a solution before the release I will be opening a revert

bnoordhuis · 2018-04-05T16:33:53Z

If gulp@3 cannot or does not want to upgrade, then it's going to break sooner or later anyway. Reverting is pointless, that's just putting off the inevitable.

devsnek · 2018-04-05T16:42:27Z

+1 to what bnoordhuis said, if we were trying to keep all packages working forever node wouldn't have versions

MylesBorins · 2018-04-05T17:37:46Z

@bnoordhuis if gulp@3 cannot upgrade and it is going to break an insignificant number of developers who are not going to understand why we should have a very very good reason to do so. While I find this PR to be a good thing, I do not find it compelling enough to justify the breakage

joyeecheung · 2018-04-05T19:25:27Z

@MylesBorins I would've agreed if this is breaking gulp@4, but I don't think Node v10 breaking gulp@3 should that blocking. This is putting a burden both on Node and gulp. If the user cannot upgrade to gulp@4, then it's reasonable to expect things would stop working on a new major version of Node. People who have to stay with an older version of gulp can also stay with an older of version of Node to be safe.

addaleax · 2018-04-05T19:48:18Z

Just for context, gulp@4 is only a pre-release at this point. But yes, I agree, if gulp doesn’t want to be broken, then at some point they’re just going to have to release a version of gulp that uses public, semver-covered APIs… 2 years is plenty of time to do that.

phated · 2018-04-05T20:17:01Z

This thread is absolutely ridiculous. Many of your are literally paid to work on this stuff and many others have jobs that allow you to work on it (on or off time). I'm getting blown up by this thread in the middle of interviews and yet none of you have offered to help write docs or get your companies to contribute funding for the project. I've given 2 years to this project, without a job, unpaid and now you demand me to do more work for free so you won't break our publicly available and documented project or expect me to push out an un/under-documented version? I seriously can't believe this.

joyeecheung · 2018-04-05T20:28:34Z

@phated I am sorry about what you've been through but the amount of paid labour in Node core is probably much less that you thought. In case you didn't know, at least three Node.js collaborators on this thread are students. By referring to gulp, we do not mean you personally, but the project in general. Node.js changes breaking unmaintained packages is not uncommon and I'll have to say, if things are just unsustainable, it's fine to drop things and move on. By keeping undermaintained pakcages working with newer version of Node probably would do more harm to the ecosystem in the long run.

phated · 2018-04-05T20:35:45Z

@joyeecheung the project is not unmaintained - it's just undermaintained (by only me). ~~Asking~~ nay, telling me to ship an underdocumented version should be seen untenable to the larger community. Your whole attitude towards gulp makes me feel like you are the "gulp's dead, use webpack" train that isn't even true from the broader developer ecosystem.

The pay is not "much less than I thought", as I'm guessing I have much more insight into companies/politics/etc. If you are an unpaid person working on node core, you should really take a step back and realize how much money people are making off your back.

addaleax · 2018-04-05T20:37:42Z

@phated Serious question: If somebody here prepared an alternative gulp@4 release that differs in no way from gulp@3 except using graceful-fs@4 under the hood, would you be willing push that to npm and make gulp@5 the new gulp@4 (again, also assuming that one of us would open PRs for the relevant doc changes)?

But yes, we do respect that you are already putting a ton of work into gulp and we do know that Node.js is special in the sense that some people are getting paid for the open source work that they are doing. And we certainly don’t want to break gulp, or put it in a bad light – I’m really doing my best to keep natives, and with it graceful-fs@3 working as long as that’s feasible.

Your whole attitude towards gulp makes me feel like you are the "gulp's dead, use webpack" train that isn't even true from the broader developer ecosystem.

This might just be me, but I don’t think anybody here has an opinion like that.

phated · 2018-04-05T20:43:42Z

@addaleax We can't do your suggestion because gulp 4 is published on npm as the next tag. The thing holding it back from latest is that tons of documentation work needs to be done. I certainly don't enjoy writing docs so it is either going to take time, money or contributors (none of which we seem to have) to get it done. I don't feel comfortable shipping a version of gulp 4 that has incomplete/wrong/misleading docs because that's just going to put yet more burden/stress upon me to handle all the issues and questions that get raised (right now it's used by the hardcore that can figure it out themselves).

This might just be me, but I don’t think anybody here has an opinion like that.

Sorry, I meant that in relation to @joyeecheung's comment about it being unmaintained - especially because I've been maintaining it daily until very recently.

yocontra · 2018-04-05T20:48:27Z

I'll reiterate that I think the best (and most correct) solution here is for a non-breaking graceful-fs v3 patch to be published. As long as the API has no breakages, I don't understand how swapping the underlying native library is impossible do to as a patch. I'm willing and able to do whatever is necessary to patch/whatever on our end if need be but a new major release it out of the question.

As an aside, we really don't need to be discussing gulp, our project, our maintainers, etc. - not really relevant, thanks for worrying about us but we're okay thanks. Let's leave that out of this and just focus on fixing this please.

benjamingr · 2018-04-05T21:09:40Z

Can't graceful-fs just be updated in a patch version in the Gulp 3 branch?

phated · 2018-04-05T21:19:16Z

@benjamingr that's not how semver works. Sorry.

joyeecheung · 2018-04-05T21:22:25Z

@joyeecheung the project is not unmaintained - it's just undermaintained (by only me). Asking nay, telling me to ship an underdocumented version should be seen untenable to the larger community. Your whole attitude towards gulp makes me feel like you are the "gulp's dead, use webpack" train that isn't even true from the broader developer ecosystem.

Sorry if I came across that way, I was aware that it is still maintained at the moment, but your comments sounded like the maintenance is unsustainable in the sense that the maintainer is not paid and is not in a situation where that is OK, which means it is possible to go unmaintained, even if that would be very unfortunate. By saying Node.js changes breaking unmaintained packages is not uncommon, I was not talking about gulp as it currently is. I was talking about our approach on breaking unmaintained packages in general. I respect your work on gulp and I hope it does not go unmaintained, of course, and I certainly recognize that it is unfair to expect packages get maintained automatically, but I also think it is unfair to expect Node.js core to maintain that level of compatibility - compatibility of undocumented internals - automatically.

I was not telling you to ship an underdocumented version, and again, sorry if I came across that way. If I understand the situation correctly, it's not urgent that gulp@4 must be shipped as soon as Node v10 is out - gulp@3 still works on multiple active release lines of Node.js. Things might be urgent once all the active release lines go EOL but by then gulp@4 may not be underdocumented anymore. Also to my understanding a lot of users don't even start to adopt an even-numbered version until it goes LTS.

If you are an unpaid person working on node core, you should really take a step back and realize how much money people are making off your back.

Well, I am writing in the middle of the night during a national holiday..probably says a lot about me.

@contra

As an aside, we really don't need to be discussing gulp, our project, our maintainers, etc. - not really relevant, thanks for worrying about us but we're okay thanks. Let's leave that out of this and just focus on fixing this please.

I will recuse myself from further commenting on those issues, sorry.

addaleax · 2018-04-05T21:52:28Z

@phated People generally do perform semver-major updates of dependencies in patch releases if that doesn’t break the dependent module, so I guess it might be helpful to ask: In what way does gulp proxy the graceful-fs API? I’ve only used gulp a few times, but I don’t remember seeing anything like that…

yocontra · 2018-04-05T22:16:00Z

@addaleax The issue is that graceful-fs v4 contains more changes than just this fix, most importantly the change from patching the fs module to being isolated (a good move, but breaking nonetheless).

Our code doesn't rely on this behavior, but our users (especially windows users, where fs is a mess) rely on that behavior inadvertently. Some gulp plugins use the fs module directly, some gulp users use the fs module directly in their gulpfiles - and right now (on v3) they are getting a patched graceful-fs module which is silently solving problems for them. If we backport graceful-fs@4 to gulp@3 they won't be getting that anymore, and then we're on github issues for a few weeks handling hundreds of tickets from windows users for EMFILE errors - and all of this breakage on a patch release nonetheless. I am personally 100% opposed to overriding globals, but this is what graceful-fs was doing and now people have code in the wild that relies on this.

The last fix for this was backported to v3 here - isaacs/node-graceful-fs@331ac8d so if the natives module updates then problem solved. If resources is an issue I will help with the update, just let me know. This seems to be the simplest and most direct approach here. I don't think we should be maintaining hacks forever but this is a particularly awkward situation node core has put everyone in, and I would rather just resolve it than continue discussing for weeks on end.

MylesBorins · 2018-04-05T22:55:29Z

I was not telling you to ship an underdocumented version, and again, sorry if I came across that way. If I understand the situation correctly, it's not urgent that gulp@4 must be shipped as soon as Node v10 is out - gulp@3 still works on multiple active release lines of Node.js. Things might be urgent once all the active release lines go EOL but by then gulp@4 may not be underdocumented anymore. Also to my understanding a lot of users don't even start to adopt an even-numbered version until it goes LTS.

I have an alternative view on this. Once 10.x goes into LTS it will be the primary link on the website of where we drive download. Gulp has close to 1M downloads a week and is embedded in TONS of tools which are not necessarily going to get an upgrade in the near future.

This is setting people up for failure.

The real issue here is not node, gulp, vinyl-fs, graceful-fs... but the plethora of tools sitting on top of that stack and the arguably millions of people relying on those to keep working the way they expect it to. Those are the individuals we need to be focused on supporting, collectively.

I still have not seen a response in this thread about why this particular change is time sensitive and important enough to break such a widely used tool in our ecosystem in an LTS release. At the very least we can kick this off by 6 months and try to actively work towards making a better solution.

This is not too different from the issue we have seen with buffer constructor in that each Semver-Major this issue rears it's head and makes our lives difficult. We've applied different fixes, but have not found a way to fix this permanently.

My personal feeling is that if we can't land this in a way that doesn't break gulp 3 it shouldn't land in a branch that will go LTS, full stop. We can always keep this on master and actively work towards fixing the breakage before we cut 11... I am not sure what "fix" means... but it is obvious that we all need to work together and be strategic about this.

phated · 2018-04-05T23:02:06Z

@MylesBorins ❤️ ❤️ ❤️ ❤️ ❤️

devsnek · 2018-04-05T23:26:17Z

I have an alternative view on this. Once 10.x goes into LTS it will be the primary link on the website of where we drive download. Gulp has close to 1M downloads a week and is embedded in TONS of tools which are not necessarily going to get an upgrade in the near future.
This is setting people up for failure.

node code gets left behind in the dust all the time, otherwise we would still have api compatability with node 0.10. if it makes people super uncomfortable to be updating internals that older package release rely on we can put up a notice "this release changed stuff with natives that really big modules like gulp depend on etc etc feel free to stop by the gulp issue tracker to help them reach version 4 and by the time node 10 is LTS the issue should have solved itself. any individuals or companies with their own internal gulp setups should know what a major node release means and that packages on npm published before that version of node was released might not work.

jasnell · 2018-04-05T23:31:14Z

@MylesBorins, I get where you're coming from but there's an alternative view here that needs to be considered: any code that is knowingly or unknowingly using a third-party modified version of a core module is most likely going to break no matter what we do and could break at any time. Even semver-patch and semver-minor changes could be breaking in such cases. We saw that happen with the introduction of and movement of code to internal/*, which, by all counts, should have been completely transparent to user code.

Use of and dependency on modules like graceful-fs and natives -- modules that knowingly do things they are explicitly warned against doing -- is really the thing that is setting downstream users up for failure.

The Buffer constructor issue is different. That is a situation directly caused by poor design and implementation decisions made in Node.js core. We are directly responsible for the userland impact that decision and the subsequent deprecation/replacement strategy has had on the ecosystem. That is fundamentally different than the situation with graceful-fs and natives. I'm all for maintaining as much stability as we possibly can, but building a dependency on code in a manner that the developers know is unsupported is not sustainable.

10.0.0 is not an LTS release. We have six months until 10.x becomes an LTS release. We have reverted semver-majors within LTS-bound release streams before if the ecosystem troubles become too great of a burden. I'm -1 on reverting this in 10.0.0 but I'm open to considering a revert before 10.x goes into LTS. In the meantime, let's see if we can get gulpjs more support in getting v4 out the door.

addaleax · 2018-04-05T23:52:26Z

My personal feeling is that if we can't land this in a way that doesn't break gulp 3 it shouldn't land in a branch that will go LTS, full stop. We can always keep this on master and actively work towards fixing the breakage before we cut 11... I am not sure what "fix" means... but it is obvious that we all need to work together and be strategic about this.

@MylesBorins I don’t know if your comment implies something else, but this particular issue is fixed on the graceful-fs/natives side.

MylesBorins · 2018-04-06T00:39:20Z

@jasnell I think that is a reasonable compromise, although I would like to see if we can get a temporary fix in before 10.0.0 and am willing to spend some time trying to help out with it. Although it seems like @addaleax is implying that this has already been fixed... so I am a bit confused. I likely should boot some things up and see what breaks 😄

@addaleax by "Fix" I meant long term solution... I don't know what that is.

yocontra · 2018-04-06T00:41:08Z

@addaleax If the issue is fixed in natives is there a problem then? Assuming this continues to work this should be fine to hold us over until gulp 4 comes out. I just installed gulp and got natives@1.1.3 which seems to be the latest. Thank you for updating that, btw.

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Mar 16, 2018

devsnek reviewed Mar 16, 2018

View reviewed changes

bnoordhuis reviewed Mar 16, 2018

View reviewed changes

targos force-pushed the error-codes-contextify branch from f2091f7 to 107feca Compare March 17, 2018 10:17

targos force-pushed the error-codes-contextify branch 2 times, most recently from a4f60a4 to 3233dcf Compare March 22, 2018 15:47

targos commented Mar 22, 2018

View reviewed changes

lib/vm.js Outdated

}

const {

displayErrors = true,

breakOnSigint = false

Copy link

Member Author

targos Mar 22, 2018

Choose a reason for hiding this comment

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

ditto: I'd like to validate the type of all options

targos force-pushed the error-codes-contextify branch from c42dcdd to 8ddc3a2 Compare March 22, 2018 16:05

joyeecheung added the errors Issues and PRs related to JavaScript errors originated in Node.js core. label Mar 22, 2018

joyeecheung reviewed Mar 22, 2018

View reviewed changes

targos force-pushed the error-codes-contextify branch from 2626409 to c0cea8a Compare March 29, 2018 05:20

jasnell approved these changes Mar 29, 2018

View reviewed changes

joyeecheung approved these changes Mar 30, 2018

View reviewed changes

mcollina mentioned this pull request Mar 30, 2018

update to gulp@4 or remove gulp nearform/nf-present#8

Closed

bnoordhuis mentioned this pull request Apr 9, 2018

CHECK fails when using natives on node >= 8 #19891

Closed

vm: move options checks from C++ to JS #19398

vm: move options checks from C++ to JS #19398

Conversation

targos commented Mar 16, 2018 • edited

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TimothyGu commented Mar 17, 2018

targos commented Mar 17, 2018

targos commented Mar 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

targos commented Mar 22, 2018

joyeecheung left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joyeecheung Mar 23, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

targos commented Mar 23, 2018

targos commented Mar 29, 2018

targos commented Mar 29, 2018

targos commented Mar 29, 2018

targos commented Mar 30, 2018

mcollina commented Mar 30, 2018

bnoordhuis commented Mar 30, 2018

addaleax commented Mar 30, 2018

mcollina commented Mar 30, 2018

targos commented Mar 30, 2018

mcollina commented Apr 5, 2018

jdalton commented Apr 5, 2018 • edited

yocontra commented Apr 5, 2018 • edited

mcollina commented Apr 5, 2018

jasnell commented Apr 5, 2018

MylesBorins commented Apr 5, 2018

bnoordhuis commented Apr 5, 2018

devsnek commented Apr 5, 2018

MylesBorins commented Apr 5, 2018

joyeecheung commented Apr 5, 2018 • edited

addaleax commented Apr 5, 2018

phated commented Apr 5, 2018 • edited

joyeecheung commented Apr 5, 2018 • edited

phated commented Apr 5, 2018

addaleax commented Apr 5, 2018

phated commented Apr 5, 2018

yocontra commented Apr 5, 2018

benjamingr commented Apr 5, 2018

phated commented Apr 5, 2018

joyeecheung commented Apr 5, 2018 • edited

addaleax commented Apr 5, 2018

yocontra commented Apr 5, 2018 • edited

MylesBorins commented Apr 5, 2018

phated commented Apr 5, 2018

devsnek commented Apr 5, 2018 • edited

jasnell commented Apr 5, 2018

addaleax commented Apr 5, 2018

MylesBorins commented Apr 6, 2018

yocontra commented Apr 6, 2018 • edited

targos commented Mar 16, 2018 •

edited

joyeecheung Mar 23, 2018 •

edited

jdalton commented Apr 5, 2018 •

edited

yocontra commented Apr 5, 2018 •

edited

joyeecheung commented Apr 5, 2018 •

edited

phated commented Apr 5, 2018 •

edited

joyeecheung commented Apr 5, 2018 •

edited

joyeecheung commented Apr 5, 2018 •

edited

yocontra commented Apr 5, 2018 •

edited

devsnek commented Apr 5, 2018 •

edited

yocontra commented Apr 6, 2018 •

edited