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

master #3

Closed
wants to merge 2 commits into from
Closed

master #3

wants to merge 2 commits into from

Conversation

gayratv
Copy link

@gayratv gayratv commented Mar 22, 2021

add cloneReadableStream.ts with new ES6 typescript syntax for Node 14 and 15

Copy link
Owner

@levansuper levansuper left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First of all thanks for the PR, I really appreciate it.

Could you briefly explain the changes and the problems they solve?

readClone1.pipe(writeStream1);
readClone2.pipe(writeStream2);

console.log('File test.txt was copied - OK.');
Copy link
Owner

@levansuper levansuper Mar 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to stick to dash-case while naming the files and name the test files as clone-readable-stream.spec.ts
Also could we have a separate test folder for all the test files?

_read() {}
}

module.exports = ReadableStreamClone;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dash-case for file names please

"singleQuote": true,
"printWidth": 120,
"tabWidth": 2
}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we use "useTabs": true instead of spaces, please

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm push new commit, please - see :)

@gayratv
Copy link
Author

gayratv commented Mar 24, 2021

First of all thanks for the PR, I really appreciate it.

Could you briefly explain the changes and the problems they solve?

First of all thanks for the PR, I really appreciate it.

Could you briefly explain the changes and the problems they solve?

First of all, I would like to express my gratitude to you for your wonderful package.

This is my first PR - so I apologize for any possible mistakes :)

Your package works great on Node 14LTS, so there are no problems to solve.

But: the documentation says:
https://nodejs.org/dist/latest-v14.x/docs/api/util.html#util_util_inherits_constructor_superconstructor

"Usage of util.inherits () is discouraged. Please use the ES6 class and extends keywords to get language level inheritance support."

Actually, only for this reason, I decided to use ES6 syntax and rewrite the code a little.

@levansuper
Copy link
Owner

Closing this due to #6.
Maybe we could work on #10 instead

@levansuper levansuper closed this Feb 28, 2024
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.

3 participants