Replace @ljharb/through with native solution #55
Conversation
|
@feross Any comments on this PR? Judging by emotes, there is a clear demand from the community for these kind of changes. |
|
@feross ping?.. |
|
@feross If you don't believe that this PR is useful, can we get at least a comment stating so?.. |
| @@ -2,7 +2,7 @@ | |||
| var fs = require('fs') | |||
| var once = require('once') | |||
| var split = require('split') | |||
| var through = require('@ljharb/through') | |||
| var { Writable } = require('node:stream') | |||
There was a problem hiding this comment.
use of destructuring, or node:, would be a breaking change (CI support has no bearing on what engines are supported; only engines.node, which defaults to *, does)
| var { Writable } = require('node:stream') | |
| var Writable = require('stream').Writable |
also, Writable wasn't added until node 0.10, which technically would also be a breaking change and require an engines.node declaration to be added.
There was a problem hiding this comment.
chalk 4 requires node 10+, so older versions aren't supported already.
and CI for hostile is only running on Node 14, so there was no indication for old version support to begin with.
There was a problem hiding this comment.
indeed you're right; 6d5d1c3 was a breaking change in a patch version.
Again, lack of CI coverage indicates nothing; engines.node combined with "what actually worked at one point within the major" is the indicator. In other words, chalk needs to be dropped to 3 or replaced, and CI coverage should be expanded.
There was a problem hiding this comment.
@ljharb literally nobody complained over all this time, so looks like it's not a problem for anyone among real users of the library, so why fix something that isn't broken?
There was a problem hiding this comment.
being semver-compliant requires adhering to principle, regardless of whether there's actual/reported breakage or not.
It's up to the maintainer, of course, whether they want to comply with semver or not - I'm just helpfully pointing out what that requires.
There was a problem hiding this comment.
@feross what is your stance on this? if requested, I can do the obsolete Node compatibility changes in this PR, but it seems more reasonable for me to stay on Node 10+
| @@ -33,7 +33,12 @@ exports.getFile = function (filePath, preserveFormatting, cb) { | |||
| cb = once(cb) | |||
| fs.createReadStream(filePath, { encoding: 'utf8' }) | |||
| .pipe(split()) | |||
| .pipe(through(online)) | |||
| .pipe(new Writable({ | |||
| write: (chunk, encoding, callback) => { | |||
There was a problem hiding this comment.
as would arrow functions
| write: (chunk, encoding, callback) => { | |
| write: function (chunk, encoding, callback) { |
There was a problem hiding this comment.
see above, realistically hostile already requires node 10+ due to chalk usage
Considering that CI is only executed on Node 14, it can be implied that very old Node.js version are not supported by this library.
Therefore it it possible to benefit from using native Node.js functionality and remove the unnecessary dependency.