Skip to content

Do not rely on Node.js internals#296

Closed
mcollina wants to merge 4 commits intogulpjs:masterfrom
mcollina:fix-node-9
Closed

Do not rely on Node.js internals#296
mcollina wants to merge 4 commits intogulpjs:masterfrom
mcollina:fix-node-9

Conversation

@mcollina
Copy link
Copy Markdown
Contributor

@mcollina mcollina commented Jan 17, 2018 •
edited
Loading

This restores compatibility with Node 9 by lifting the code from Node 8.9.4 and copying and pasting it here. This should have been the approach from the very beginning, because vinyl-fs is creating its own WriteStream that has very little in common with the one from Node core.

Unfortunately, pulling functions from a prototype and attaching them to a non-related one is a very dangerous technique: as you can imagine a function attached to a prototype relies on the state that is created by the relative constructor. These are implementation details, and they can surely change across major versions.

As a side note, this was likely broken by nodejs/node#15407 and nodejs/node#18002.

Fixes #295

The actual logic can probably be simplified, because vinyl-fs WriteStream implementation is different than the one in Node core.

mcollina referenced this pull request in gulpjs/gulp Jan 17, 2018
@phated
Breaking: Update dependencies & increase alpha version
99bdc9e
@phated
Copy link
Copy Markdown
Member

phated commented Jan 18, 2018

@mcollina thanks! It looks like these changes devastated Windows. Any ideas?

@mcollina
Copy link
Copy Markdown
Contributor Author

mcollina commented Jan 18, 2018

@phated I've just checked on a Windows box, and it's failing even on master with Node 8 and 6. It's not related (but should probably be fixed anyway in another PR).

@phated
Copy link
Copy Markdown
Member

phated commented Jan 18, 2018

@mcollina not sure about that https://ci.appveyor.com/project/gulpjs/vinyl-fs/build/735 - we've had successful builds for quite some time.

@mcollina
Copy link
Copy Markdown
Contributor Author

mcollina commented Jan 18, 2018

@phated these are what I am getting (more or less all of the same, ENOENT on files):

  24) symlink stream (windows) options can disable junctions for a directory (as a function):
     Uncaught Error: ENOENT: no such file or directory, stat 'C:\Users\Matteo\Repositories\vinyl-fs\test\out-fixtures\foo'
      at Error (native)
      at Object.fs.statSync (fs.js:1009:11)
      at Object.statSync (node_modules\graceful-fs\polyfills.js:297:22)
      at assert (test\symlink.js:395:22)
      at ConcatStream.<anonymous> (node_modules\concat-stream\index.js:36:43)
      at finishMaybe (node_modules\readable-stream\lib\_stream_writable.js:607:14)
      at endWritable (node_modules\readable-stream\lib\_stream_writable.js:615:3)
      at ConcatStream.Writable.end (node_modules\readable-stream\lib\_stream_writable.js:571:41)
      at ConcatStream.Writable._destroy (node_modules\readable-stream\lib\_stream_writable.js:662:8)
      at ConcatStream.destroy (node_modules\readable-stream\lib\internal\streams\destroy.js:36:8)
      at node_modules\mississippi\node_modules\pump\index.js:43:45
      at call (node_modules\mississippi\node_modules\pump\index.js:50:3)
      at Array.forEach (native)
      at node_modules\mississippi\node_modules\pump\index.js:70:25
      at f (node_modules\once\once.js:25:25)
      at Pumpify.<anonymous> (node_modules\mississippi\node_modules\pump\index.js:29:21)
      at Pumpify.f (node_modules\once\once.js:25:25)
      at Pumpify.onerror (node_modules\end-of-stream\index.js:43:12)
      at Pumpify.Duplexify._destroy (node_modules\duplexify\index.js:195:15)
      at node_modules\duplexify\index.js:179:10
      at _combinedTickCallback (internal/process/next_tick.js:73:7)
      at process._tickCallback (internal/process/next_tick.js:104:9)

(this is node 6 on master)

Maybe a dependency changed and broke things?

@mcollina
Copy link
Copy Markdown
Contributor Author

mcollina commented Jan 18, 2018

Can you trigger a build on master?

@phated
Copy link
Copy Markdown
Member

phated commented Jan 18, 2018

@mcollina done and still passing on Windows: https://ci.appveyor.com/project/gulpjs/vinyl-fs/build/738

@mcollina
Use Writable instead of FlushWriteStream
53a8109 
Since version 2.3.0, readable-stream offers _final in Writable.
@mcollina
Add Node 9 to appveyor.
676d868
@mcollina
Copy link
Copy Markdown
Contributor Author

mcollina commented Jan 19, 2018

@phated the test suite on master fails on Windows Home Edition. I guess it's due to the permissions required to create symlinks (I stumbled upon that in the past, and it seems the case).

I ended up removing FlushWriteStream and using Writable directly (since readable-stream 2.3.0 it offer _final). The alternative is to copy the methods from Node 6 instead, those should work. I went for Writable, because it removes a dependency.
Let me know if you prefer the Node 6 approach.

@phated
Copy link
Copy Markdown
Member

phated commented Jan 22, 2018

I ended up removing FlushWriteStream and using Writable directly (since readable-stream 2.3.0 it offer _final)

Oh wow, so _final is always called before the defined .close method is called (since that closes the file descriptor)? That's a great additional API!

@mcollina
Copy link
Copy Markdown
Contributor Author

mcollina commented Jan 22, 2018

Oh wow, so _final is always called before the defined .close method is called (since that closes the file descriptor)? That's a great additional API!

Yes! _final is called before the streams emits 'finish', it was such a useful feature that we added it in Node 8 (readable-stream@2.3.x).

@phated
Copy link
Copy Markdown
Member

phated commented Jan 23, 2018

This was rebased and merged as 167d8ff and 62d1969 - publishing as 3.0.2 once CI completes.

Thanks again!

@phated phated closed this Jan 23, 2018
@mcollina mcollina deleted the fix-node-9 branch January 31, 2018 09:56
@mcollina mcollina mentioned this pull request Feb 27, 2018
Closed
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@mcollina @phated