Repository navigation
Unbreak gulp 3 for Node 10 #19786
Description
Activity
Sigh.
graceful-fs@3using internals strikes back again…gulp@3usesgraceful-fsv3 branch which seems to be usingnatives@^1.1.0.
So, if we could fix that onnativesside, it should fix things for gulp and everyone else usinggraceful-fs@3.Note:
graceful-fs@4is fine already, only@3and 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
contexifybindings are still available.
What is the issue thatnativesis 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.Reacted by Jens Hausdorf, snek, Nikita Skovoroda, Alexandru Dima, Sen, Ivan Kolmychek, Peter Richardson, Jason Allen, Hugo Xu, Oleg Malakotin and 3 moreAh great!
So
ContextifyScriptis 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
ContextifyScripton 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
Reacted by Jens Hausdorf, Arnaud Benhamdine and ExE Boss@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 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 isgulp. Thenunzip, then gulp-dependent modules start, thens3, thenunzip2, 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
nativeswere 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 breakgulpitself. 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-fsandnativesissues further, feel free to open a new ticket. This should not block Node 10 anymore.@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
nativesversion (also deprecated) that would only export the API that graceful-fs@3 uses and nothing else, to avoid new users relying on it.@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, orspdy, for example)One thing that we should probably do after the 10.x release is migrating more
process.binding()code tointernalBinding, that should be a really good step in the right directionReacted by Joyee Cheung and ExE BossOne 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 directionPlease 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, 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
nativesfix reliable enough or should Gulp actively try to backportgraceful-fs@4support to Gulp 3?@addaleax has stated that
nativesis not a viable long-term solution, with a lifespan better measured in months than years. If I were gulp, I'd look into upgrading.Reacted by Nikita Skovoroda, Anna Henningsen and Ivan KolmychekGulp 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
Reacted by Michael Cheung, Ivan Kolmychek, Oleg Malakotin, Albert Sanchez and Ruslan StelmachenkoUpdating
graceful-fsis a breaking change so it will not be fixed ingulp@3, they'll have to update togulp@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.- added 2 commits that reference this issue
on May 26, 2018 - added a commit that references this issue
on Dec 12, 2018 - added a commit that references this issue
on Nov 23, 2020 - added a commit that references this issue
on Jul 4, 2023 - added a commit that references this issue
on Jul 12, 2024
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:
nativesmodule makes a direct use of our internalcontextifybinding. We can probably fix that up.natives. That PR is an important step towards having error codes for all errors in Node./cc @addaleax