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

fix: update readSync to not provide seek / offset parameters #159

Merged
merged 1 commit into from
Dec 18, 2023

Conversation

trgwii
Copy link
Contributor

@trgwii trgwii commented Nov 22, 2023

Something in the internals of fs.readSync changed at some point, which causes the following error if you provide an offset integer to readSync while reading stdin:
ESPIPE: invalid seek, read
Solution is to not provide additional arguments, as the defaults are equivalent for this usecase.
Tested on Debian 11 and Windows 10

Closes #158

@uncenter
Copy link
Contributor

uncenter commented Nov 27, 2023

I mean the defaults don't look equivalent; 0, 1, 0 vs 0, buf.byteLength, null. This does fix my issue but I want to make sure this won't break other things :)

@trgwii
Copy link
Contributor Author

trgwii commented Nov 29, 2023

Unless this breaks on macOS, I don't think there are other cases to consider, as readlineSync is only used by the prompt / confirm builtins, the only usecase is synchronously fetching an answer from a user on stdin.

@uncenter
Copy link
Contributor

Working for me on macOS!

@uncenter
Copy link
Contributor

uncenter commented Dec 12, 2023

Any update on this and #160? Both look good to merge to me.

@trgwii
Copy link
Contributor Author

trgwii commented Dec 12, 2023

For the record, this should be a nonbreaking change for older node versions as well, since I just omit optional parameters here.

@dsherret dsherret changed the title Update readSync to not provide seek / offset parameters, closes #158 fix: update readSync to not provide seek / offset parameters Dec 18, 2023
Copy link
Member

@dsherret dsherret left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks! I will merge this tomorrow along with the other PRs then do a release then.

@dsherret dsherret merged commit 12022d3 into denoland:main Dec 18, 2023
4 checks passed
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.

@deno/shim-prompts prompt doesn't end/exit on enter
3 participants