Unbreak gulp 3 for Node 10

[targos]

Since #19398 landed, gulp 3 (as well as any other package that uses an old version of graceful-fs) is broken on master. It is a very popular tool (4M downloads/month) and while there is a version 4 that should not be affected, version 3 is still tagged as "latest" on npm.

We think it's important to fix this and there are two possible ways:

1. Fix the source of the issue: The natives module makes a direct use of our internal contextify binding. We can probably fix that up.
2. Revert vm: move options checks from C++ to JS #19398 in Node 10. This should if we end up being unable to fix natives. That PR is an important step towards having error codes for all errors in Node.

/cc @addaleax

[ChALkeR]

Sigh. graceful-fs@3 using internals strikes back again…

gulp@3 uses graceful-fs v3 branch which seems to be using natives@^1.1.0.
So, if we could fix that on natives side, it should fix things for gulp and everyone else using graceful-fs@3.

Note: graceful-fs@4 is fine already, only @3 and below abused internals, it was fixed by @bnoordhuis rewriting it to a clean approach.

[jdalton]

The natives module makes a direct use of our internal contextify binding. We can probably fix that up.

It looks like contexify bindings are still available.
What is the issue that natives is having with them?

[jasnell]

I'm generally -1 on reverting the commit in Node.js 10. The proper fix is to address the issue in natives. We have to be able to evolve and improve Node.js internals without being held hostage by older modules that were doing things they were repeatedly warned not to do.

[targos]

@jdalton The signature of the methods changed, for example from new ContextifyScript(code, options) to new ContextifyScript(code, filename, lineOffset, columnOffset, cachedData, produceCachedData, parsingContext)

[jdalton]

Ah great!

So ContextifyScript is a native method... could simply wrapping it in JS tackle it?

Something like:

const RealContextifyScript = ContextifyScript

ContextifyScript = function (code, filename, lineOffset, columnOffset, cachedData, produceCachedData, parsingContext) {
  if (arguments.length < 3 && isObject(filename)) {
    ...
  }
  ...  
  return new RealContextifyScript(...)
}

then bolt the wrapped ContextifyScript on to the binding object and wire up constructor prototypes as needed.

[devsnek]

how about we just put a message in the changelog

Please note that changes to node internals were made which will affect older versions of graceful-fs, vinyl-fs, natives, etc. Use of these versions is obviously not recommended and if your favourite project is still using them consider opening a pr to update it.

many many people using gulp and updating to node 10 will see that, someone will open a pr, by the time LTS is out gulp 3 would probably have some sort of patch or maybe gulp 4 will finish their docs and stuff. if that doesn't happen, people should look for another build system. this is why we have major versions of node; things will break, and the ecosystem's opensourceness allows broken packages to get fixed or for replacements to form

[addaleax]

This is fixed in natives, for now.

@devsnek Your argumentation really only holds for packages that adhere to semver, which also means using public API.

[ChALkeR]

@addaleax Nice, thanks!

Is there anything else left to do here?

[addaleax]

@ChALkeR I’m not sure, we might want to have a talk about how we progress with this.

It’s becoming harder to support graceful-fs@3 at a quickly increasing rate, and like @bnoordhuis said in the other thread, this is inevitably going to break completely at some point in the not-so-distant future.

[ChALkeR]

@addaleax That is true, but the immediate issue targeting 10.0.0 seems to be fixed now, correct?

As for graceful-fs@3 — the most significant consumer (by an order of magnitude) of that is gulp. Then unzip, then gulp-dependent modules start, then s3, then unzip2, and a lot more gulp-dependent modules.

[addaleax]

That is true, but the immediate issue targeting 10.0.0 seems to be fixed now, correct?

Yes, but while fixing that issue I noticed that tests for natives were failing on Node 6 & 8 as well, for different reasons each. We probably didn’t notice that because the changes on our side didn’t break gulp itself. I’m not convinced that having gulp in CITGM will catch all of the potential issues for consumers of graceful-fs@3 that we might introduce in Node.js.

[targos]

If you want to discuss graceful-fs and natives issues further, feel free to open a new ticket. This should not block Node 10 anymore.