POC: Introduce Node.js 6+ compatibility mode for gulp@3

[ChALkeR]

Refs: #1640 (comment)

This is unfinished POC, it lacks the documentation and perhaps the tests.

It works though, and successfully unbreaks gulp@3 with Node.js 6+ if gulp was required using require('gulp/compat').

This is a pure opt-in, it does not change anything unless the user specifically enables it (except for installing one extra unused package).

If gulp is already loaded at the time of the opt-in, then it throws an error — so that deps won't misuse it and change the behaviour of everything, — this makes this option available only to the top-level gulpfile itself.

If this would be accepted, I will gladly move vinyl-fs-03-compat to gulpjs both on GitHub and npm.

/cc @phated, @contra, @thealphanerd, @jasnell

Thoughts?

[MylesBorins]

Would it potentially make sense to wrap the require of graceful-fs in a try-catch. If the require fails then opt to using the compat version.

This could be a good way to allow for a transparent fix for those on affected versions.

[phated]

I find this an interesting workaround. Let me think on it for awhile.

[ChALkeR]

@phated Would it be ok to transfer vinyl-fs-03-compat ownership now? Asking in advance because I don't want to spam you =).

[phated]

@ChALkeR no worries. If we decide to pull this workaround in, I'll create a branch/tag in vinyl-fs proper

[ChALkeR]

@phated Ah, ok. That makes sense =).

[jasnell]

just fyi, related to this: nodejs/node#8166. It doesn't solve the issue but it's one small step.

[ChALkeR]

Another note: this is not a separate module, but a mere separate entry point for these reasons:

1. That requires less changes for the users to integrate, it's a one-liner this way.
2. If any dependencies used in the gulpfile also do require('gulp') on their own for some reason — this top-level opt-in would propogate to those. No changes are required from those dependencies.
3. Introducing a separate module would likely raise more concerns and recelive less usage.
4. It would be harder to support if this was a separate package.

[ChALkeR]

Ah, this approach has a drawback. Fixable, though.

The propogation would only work if the gulp loaded by require('gulp') is the exact same package, not a separate copy in a deeper node_modules dir. This could be fixed by making the compat flag in vinyl-fs-wrap.js stored per-process (e.g. in global), not per-gulp-module.

Thoughts?

[mgol]

The propogation would only work if the gulp loaded by require('gulp') is the exact same package, not a separate copy in a deeper node_modules dir.

Is this even supported by task managers like Gulp?

This could be fixed by making the compat flag in vinyl-fs-wrap.js stored per-process (e.g. in global), not per-gulp-module.

Had this been necessary, could we use a symbol to minimize the possibility of a collision?

[ChALkeR]

@mgol

Is this even supported by task managers like Gulp?

I could definitely imagine people doing require('gulp') in some package out there for some reason (perhaps even by a mistake). And I could imagine such package (or it's dependant of some level) being required from the gulpfile, for some reason. It doesn't even matter if that code path that actually uses the require('gulp') result would be not taken in the dependency — require('gulp') would crash itself.

Note that about 2943 packages depend on gulp via the dependencies chain (even without the devDependencies). That's about 1% of all npm packages.

[ChALkeR]

@mgol e.g. sifttt is both managed using a gulpfile with a top-level require('gulp') and does require('gulp') inside the package itself: markbirbeck/sifttt/blob/master/lib/beam/GulpSource.js#L3.

[ChALkeR]

Note: [email protected] was released by @isaacs and that version does not trigger the warning anymore, so this can be closed now.

[ChALkeR]

Closed, will reopen if any issues arise with the new graceful-fs or if the change there gets reverted for some reason.

[phated]

Works for me. Thanks to everyone that contributed towards a solution!

[ChALkeR]

For posterity and anyone else with graceful-fs@3: this was done with https://github.com/isaacs/natives/blob/master/index.js, which also uses a bunch of undocumented and internal things, relies on implementation details, and also has some other hacks. E.g. it's basically a noop on the buffer module, and that one was just the module that will definitely have issues with that approach, and other modules also could be affected.

That perhaps shouldn't manifest itself in the near future in regards to graceful-fs@3, (hence it was ok to close this now and leave things as they are), but in the long term no one should ever user graceful-fs below v4 and no one should ever use that natives module in their own projects.