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

Incomplete solution to node v10 compatibility #24

Closed
wants to merge 1 commit into from
Closed

Incomplete solution to node v10 compatibility #24

wants to merge 1 commit into from

Conversation

tmcw
Copy link

@tmcw tmcw commented May 2, 2018

This fails 3 tests in Node v10 - I can't quite find a way to keep cp-file's promise that it'll only create a directory for output if the output can be read from the stream - that requires waiting a tick between readable and pipe and that breaks behavior much more (instead of failing 3 tests, master fails 15.

Unfortunately the Node v10 changelog only says

The 'readable' event is now always deferred with nextTick.

Which doesn't clearly point to why this would break.

This was referenced May 2, 2018
@schnittstabil
Copy link
Collaborator

schnittstabil commented May 3, 2018

A more stripped-down test (without cp-file and pipe):

const fs = require('fs');
const read = fs.createReadStream(__filename);

new Promise((resolve, reject) => {
    read.on('error', err => {
        reject(err);
    });
    read.on('readable', () => {
        resolve(read);
    });
}).then(readable => {
    console.log('ok');
    readable.on('data', chunk => {
        console.log('data');
    });
}).catch(err => console.error(err));

Output:

# node < v10.0.0
ok
data

# node v10.0.0
ok

@tmcw
Copy link
Author

tmcw commented May 4, 2018

That looks like it could be a node core bug: filed an issue upstream: nodejs/node#20520

@tmcw
Copy link
Author

tmcw commented May 7, 2018

Closing in favor of #26

@tmcw tmcw closed this May 7, 2018
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