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

Use Streams3 instead of through #478

Closed
vasiliicuhar opened this issue Jul 17, 2019 · 7 comments
Closed

Use Streams3 instead of through #478

vasiliicuhar opened this issue Jul 17, 2019 · 7 comments

Comments

@vasiliicuhar
Copy link

vasiliicuhar commented Jul 17, 2019

Tape version: 4.11.0

test.createStream().on('readable', () => console.log('stream has new data'))
test('example', t => t.fail('failed'))

Expected: standard Streams3 behavior (should output 'stream has new data' in console)
Actual: empty console

The problem:
Tape uses through to create output stream in test.createStream(). This module has aged significantly and Streams3 were introduced since then (see https://nodejs.org/api/stream.html#stream_compatibility_with_older_node_js_versions for detailed description).

Solutions:
1. use stream.wrap on client side

2. return Readable stream from createStream()

  • should not break most of the apps, can be done in minor version, we may revert the changes if there will be a lot of affected users

3. deprecate createStream in favor of setOutput(dst: Writable)

  • dst can be anything that implements Writable (process.stdout, fs.createWriteStream(), Transform stream, etc)
  • flexible lifetime and error management for client code, reasonable defaults (i.e. create write stream on first output chunk)

I can start working on PR for setOutput()/ createStream() next week.
What do you think?

#338
We can see if we can handle dst stream close event instead of relying on process.on('exit, ...)

@ljharb
Copy link
Collaborator

ljharb commented Jul 17, 2019

Regarding solution options, it must work (or have a super graceful degradation) in both node and browsers, going back to super old versions of both.

Adding an alternative to createStream that achieves what you want (without deprecating createStream, necessarily) seems fine, if it can meet the above constraint.

Thoughts?

@Raynos
Copy link
Collaborator

Raynos commented Jul 17, 2019

This part of the code can be quite fragile since there's a lot of logic around "auto piping" to stdout if and only if no one is reading from the main default tape stream.

You can always use your own implementation of setOutput

function setOutput(test, dst) {
  test.createStream().pipe(dst)
}

I don't see any reason why this one line function should be in tape itself.

@vasiliicuhar
Copy link
Author

vasiliicuhar commented Jul 18, 2019

@ljharb leaving both APIs for a lifetime it is even worse, I would go with @Raynos solution.
@Raynos thanks for rising this concern, indeed with buffered streams auto-piping will be more difficult. But it won't be needed, because buffered streams are "paused" by default and do not loose any data.

What platforms do we really support?

super old versions of both

is not saying anything
Streams3 need to be polyfilled in browsers anyway, and Node supports them since >0.12 (see article)

Let's remember that if we release v5 in some future with new stream api, tape@4.x will continue to work in super old versions of both.
I am not pushing it, I just thought that we may explore this area to remove some code from core package, which is in line with keep it minimal approach

@ljharb
Copy link
Collaborator

ljharb commented Jul 19, 2019

ideally node 0.4 + IE 6; realistically node 0.6 + IE 8.

@vasiliicuhar
Copy link
Author

vasiliicuhar commented Jul 19, 2019

May I ask why this decision to support these old environments was made?
If one doesn't want to upgrade Node.js, he can use older versions of tape.

@Raynos
Copy link
Collaborator

Raynos commented Jul 19, 2019

The choice is mostly for library authors.

If you author a library instead of an application and you want the library to work on IE6+ and all versions of node then you need to use a testing library to run your tests on all those environments.

As an application author you just test against node10 and can use a testing library that supports node10 and higher.

@ljharb
Copy link
Collaborator

ljharb commented Jul 19, 2019

tape also remains the only test runner with this level of support, so it’s pretty important we retain it.

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

No branches or pull requests

3 participants