Skip to content

Replace @ljharb/through with native solution #55

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.

Sign up for GitHub

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

Open
kibertoad wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Replace @ljharb/through with native solution #55

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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')
Copy link
Contributor

@ljharb ljharb Oct 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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)

Suggested change
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.

Copy link
Author

@kibertoad kibertoad Oct 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@ljharb ljharb Oct 7, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Author

@kibertoad kibertoad Oct 7, 2024

Choose a reason for hiding this comment

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

@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?

Copy link
Contributor

@ljharb ljharb Oct 7, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Author

@kibertoad kibertoad Oct 7, 2024

Choose a reason for hiding this comment

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

@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+

var net = require('net')

var WINDOWS = process.platform === 'win32'
Expand Down Expand Up @@ -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) => {
Copy link
Contributor

@ljharb ljharb Oct 7, 2024

Choose a reason for hiding this comment

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

as would arrow functions

Suggested change
write: (chunk, encoding, callback) => {
write: function (chunk, encoding, callback) {

Copy link
Author

@kibertoad kibertoad Oct 7, 2024

Choose a reason for hiding this comment

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

see above, realistically hostile already requires node 10+ due to chalk usage

online(chunk.toString())
callback()
}
}))
.on('close', function () {
cb(null, lines)
})
Expand Down
1 change: 0 additions & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@
"url": "https://github.com/feross/hostile/issues"
},
"dependencies": {
"@ljharb/through": "^2.3.11",
"chalk": "^4.1.2",
"minimist": "^1.2.8",
"once": "^1.4.0",
Expand Down