2.3.1 broke vinyl-fs test suite

[phated]

I'm digging into this now but this is the only dependency that changed. Every now-failing test have to do with error handling, any thoughts off the top of your head?

[mcollina]

2.3.0 is working fine?

[phated]

Probably not, we had 2.2.11 in our last successful run.

[mcollina]

If 2.3.0 have the same behavior, it might be nodejs/node#13850.

[phated]

Just tested with 2.3.0 and the tests fail, so it looks like that is the breaking version. We are seeing as a transitive issue from our through2 dependency. How can we go about fixing it? Or does it need to be solved here?

[phated]

Edit: Reviewing the code linked above, Reviewing https://github.com/nodejs/node/pull/12925/files#diff-4c74bb09dc4dabfff99ec3bc35181885R98, I think it needs to look for _destroyed property, also, to handle through2's implementation.

[mcollina]

Ahum, that was not interfering with through2 when I did run the tests. Why do you think that?

I will have a look asap (probably tomorrow). Maybe we should add vinyl-fs to citgm, we could have caught this earlier.

[phated]

I'm attempting to apply your node patch to readable-stream so I can run our test suite with it incorporated.

[phated]

Last I heard, vinyl-fs was included in citgm but no one wanted to deal with making it run properly so they just flagged it as flakey...

[phated]

@mcollina your patch fixes our test failures. (I also want to note that lib/_stream_transform.js added a dependency on internal/errors which doesn't seem to be replaced by your build script).

Any idea how long this will take to get resolved? It's currently blocking the major release of vinyl-fs

[mcollina]

@nodejs/streams what do you think we should do?

IMHO we can forward-port that fix, and then wait for core to release.

[mcollina]

Can you please link the reference to internal/errors?

[phated]

@mcollina https://github.com/mcollina/node/blob/057339df174a375e73dfd7cd4d9e391f7cdf6d5b/lib/_stream_transform.js#L67 (In your fork, since that's what I built)

[mcollina]

Oh, that is part of core, that's the code we will convert. Not sure how it got there, I'll get it fixed.

[mcollina]

Asap is whar we'll do.

[phated]

Thanks!

[lrlna]

heyyyy! it seems like node-core's got a fix coming on the next release (and that's within a week or so?); i am thinking we should just wait for that ✌️

[phated]

It's really hard for me to find time to push towards our gulp4 (which requires this vinyl-fs major) release but I have some this week, so it would be really hard for me to wait until next week or later.

[mcollina]

@phated we have just released v2.3.2 with the fix.

[phated]

Thanks a ton @mcollina and team!