Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do not rely on Node.js internals #296

Closed
wants to merge 4 commits into from
Closed

Conversation

mcollina
Copy link
Contributor

@mcollina mcollina commented Jan 17, 2018

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.

@phated
Copy link
Member

phated commented Jan 18, 2018

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

@mcollina
Copy link
Contributor Author

@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
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
Contributor Author

@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
Contributor Author

Can you trigger a build on master?

@phated
Copy link
Member

phated commented Jan 18, 2018

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

Since version 2.3.0, readable-stream offers _final in Writable.
@mcollina
Copy link
Contributor Author

@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
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
Contributor Author

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
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants