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

.pipe signature change #949

Open
hfhchan-plb opened this issue Nov 21, 2024 · 7 comments
Open

.pipe signature change #949

hfhchan-plb opened this issue Nov 21, 2024 · 7 comments
Assignees
Labels

Comments

@hfhchan-plb
Copy link

hfhchan-plb commented Nov 21, 2024

Previously in 8.1.9, calling .pipe(process.stdout) would return ProcessPromise, now (8.2.2) it returns D & PromiseLike<D>. Is there any way to get the old behaviour?

@hfhchan-plb
Copy link
Author

Looks like this broke as part of #921 and #928, where the return type of .pipe() became polymorphic. Shouldn't this have been a major version change instead of minor?

@visnaut
Copy link

visnaut commented Nov 21, 2024

Looks like this broke as part of #921 and #928, where the return type of .pipe() became polymorphic. Shouldn't this have been a major version change instead of minor?

Agreed. I have a script in CI that broke because of this change.

The docs do not make reference to this change, and I believe the third example in the pipe doc won't work anymore, since I don't think you can chain them anymore.

@antongolub
Copy link
Collaborator

antongolub commented Nov 21, 2024

@visnaut

The docs do not make reference to this change, and I believe the third example in the pipe doc won't work anymore, since I don't think you can chain them anymore.

We'll update the example:

$`printf "hello"`
  .pipe($`awk '{printf $1", world!"}'`)
  .pipe($`tr '[a-z]' '[A-Z]'`)

// →
$`printf "hello"`
  .pipe`awk '{printf $1", world!"}'`
  .pipe`tr '[a-z]' '[A-Z]'`

@antongolub
Copy link
Collaborator

@hfhchan-plb ,

Is there any way to get the old behaviour?

Smth like this maybe, but I'm not sure:

const _pipe = ProcessPromise.prototype.pipe
ProcessPromise.prototype.pipe = function(...args) {
   const result = _pipe(...args)
   return result instanceof ProcessPromise ? result : this
}

@visnaut
Copy link

visnaut commented Nov 21, 2024

We'll update the example:

$printf "hello"
.pipe($awk '{printf $1", world!"}')
.pipe($tr '[a-z]' '[A-Z]')

// →
$printf "hello"
.pipeawk '{printf $1", world!"}'
.pipetr '[a-z]' '[A-Z]'

Thanks, I appreciate the reply.

Let me better articulate where I think the issue lies here: previously, I was doing the following:

const lines = await $(options)`command`.pipe(process.stdout).lines();

In other words, taking advantage of the fact that any method defined for ProcessPromise would return itself, allowing for the above chaining.

I've since updated the above to something like so:

const cli = $(options)`command`;

cli.pipe(process.stdout);

const lines = await cli.lines();

But you can see how anyone consuming the library and relying on the previous method signature to chain methods like that would be caught off guard by 8.2; hence why I agree with @hfhchan-plb that this maybe should have been a major version change.

antongolub added a commit to antongolub/zx that referenced this issue Nov 22, 2024
@antongolub antongolub added the bug label Nov 22, 2024
@antongolub antongolub self-assigned this Nov 22, 2024
@antongolub
Copy link
Collaborator

The problem is even broader, but I think it can be fixed. Stay tuned.

antonmedv pushed a commit that referenced this issue Nov 23, 2024
antongolub added a commit to antongolub/zx that referenced this issue Nov 24, 2024
antonmedv pushed a commit that referenced this issue Nov 24, 2024
antongolub added a commit to antongolub/zx that referenced this issue Nov 24, 2024
@antongolub
Copy link
Collaborator

@visnaut, @hfhchan-plb

Could you plz check if v8.2.2-dev.3b9afe1 solves your issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants