Unbreak gulp 3 for Node 10

@addaleax

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:

Fix the source of the issue: The natives module makes a direct use of our internal contextify binding. We can probably fix that up.
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

@bnoordhuis

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.

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?

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.

@jdalton

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

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.

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

@devsnek

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.

@addaleax

@addaleax Nice, thanks!

Is there anything else left to do here?

@ChALkeR

@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.

@addaleax

@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.

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.

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.

@addaleax

@addaleax natives was broken from the moment it was created (I opened some example issues back then) and is deprecated. There seems to be no significant users of it that are not consuming it through graceful-fs, so I don't think it should be supported past past graceful-fs@3 support, and see no benefit in fixing other things there. I would rather propose to release a semver-major natives version (also deprecated) that would only export the API that graceful-fs@3 uses and nothing else, to avoid new users relying on it.

@ChALkeR

@ChALkeR PRs welcome :)

But generally, the issues of people hooking into Node’s internals are much much larger… honestly, other projects lock us in in worse ways (like npm’s own zlib module, or spdy, for example)

One thing that we should probably do after the 10.x release is migrating more process.binding() code to internalBinding, that should be a really good step in the right direction

One thing that we should probably do after the 10.x release is migrating more process.binding() code to internalBinding, that should be a really good step in the right direction

Please don't forget many folks are reaching for internals because there is an API gap not being filled. As things are adjusted it would be great, for API found to be used in user-land, if public more consumer friendly forms would be provided in some way.

@jdalton

@jdalton, yes, I believe things are improving on that side, and filing specific issues about missing APIs would be helpful.
That said, in the graceful-fs case, it was abusing undocumented internals just because it could. The amazing-graceful-fs rewrite (that became graceful-fs 4) didn't use any new Node.js APIs afaik.

Is the natives fix reliable enough or should Gulp actively try to backport graceful-fs@4 support to Gulp 3?

@addaleax

@addaleax has stated that natives is not a viable long-term solution, with a lifespan better measured in months than years. If I were gulp, I'd look into upgrading.

Gulp 4 is fixed and stable (even if not actively promoted). I'll reach out to the Gulp devs to see if they can backport the fix to Gulp 3.

Edit: Gulp issue: gulpjs/gulp#2146

Updating graceful-fs is a breaking change so it will not be fixed in gulp@3, they'll have to update to gulp@4. Since the issue seems fixed for at least Node 10 and the Gulp migration is relatively easy it should leave enough time for people to update.

Support both Gulp 3.9.1 and 4.0.0

Upgrade to Gulp 4 (fixes build) (#1957)

Upgrade to Gulp 4 (fixes build) (facebookarchive#1957)

Gulp 3 does not support Node 10+

Upgrade to Gulp 4 (fixes build) (#1957)

Navigation Menu

Repository navigation

Unbreak gulp 3 for Node 10 #19786

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Unbreak gulp 3 for Node 10 #19786

Description

Activity

ChALkeR commented on Apr 4, 2018

jdalton commented on Apr 5, 2018

jasnell commented on Apr 5, 2018

targos commented on Apr 5, 2018

jdalton commented on Apr 5, 2018

devsnek commented on Apr 5, 2018

addaleax commented on Apr 5, 2018

ChALkeR commented on Apr 5, 2018

addaleax commented on Apr 5, 2018

ChALkeR commented on Apr 5, 2018

addaleax commented on Apr 5, 2018

targos commented on Apr 6, 2018

ChALkeR commented on Apr 6, 2018

addaleax commented on Apr 6, 2018

jdalton commented on Apr 6, 2018

ChALkeR commented on Apr 6, 2018

demurgos commented on Apr 8, 2018

bnoordhuis commented on Apr 8, 2018

demurgos commented on Apr 8, 2018

demurgos commented on Apr 8, 2018

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions