GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — tty: enable buffering by ronag · Pull Request #39253 · nodejs/node

Conversation

@ronag

@ronag ronag commented Jul 3, 2021 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

HWM was set to 0 which would cause e.g. stdout.write(...) to
always return false.

Partly reverts 954217a

Refs: #39246

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
bricss and mahdiamer reacted with thumbs up emoji bricss and mahdiamer reacted with heart emoji bricss and mahdiamer reacted with rocket emoji bricss and mahdiamer reacted with eyes emoji
All reactions
  • 👍 2 reactions
  • ❤️ 2 reactions
  • 🚀 2 reactions
  • 👀 2 reactions
@ronag
tty: enable buffering
1dd2f48 
HWM was set to 0 which would cause e.g. stdout.write(...) to
always return false.

Refs: nodejs#39246
@ronag
ronag requested a review from targos July 3, 2021 20:11
@github-actions github-actions Bot added lib / src Issues and PRs involving general changes in the lib/ or src/ directories. needs-ci PRs that need a full CI run. labels Jul 3, 2021
@ronag

ronag commented Jul 3, 2021

Copy link
Copy Markdown
Member Author

@targos not sure who's a good ping on this module?
mahdiamer reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ronag

ronag commented Jul 3, 2021

Copy link
Copy Markdown
Member Author

@nodejs/streams
mahdiamer reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ronag
fixup
48500b4
@wa-Nadoo

wa-Nadoo commented Jul 3, 2021

Copy link
Copy Markdown

I think the highWaterMark removal from is_main_thread.js (954217a#diff-c09055446adf1d69b81595307e964da2121690a1d1bf2121845c9fcac0d86c86L152) should be reverted too. It was added years ago (bb56dcc#diff-36b7877cd8c72d2e5aa27243c142181d67d7be164927bae7503af6a720203aaeR500), and its removal can be pretty breaking.
ronag reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ronag
fixup
6731cde
@ronag ronag added review wanted PRs that need review. stream Issues and PRs related to Node.js streams. and removed lib / src Issues and PRs involving general changes in the lib/ or src/ directories. needs-ci PRs that need a full CI run. labels Jul 4, 2021
@nodejs-github-bot

nodejs-github-bot commented Jul 4, 2021

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/38847/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ronag ronag added dont-land-on-v12.x labels Jul 4, 2021
addaleax
addaleax reviewed Jul 4, 2021
View reviewed changes
Comment thread lib/internal/bootstrap/switches/is_main_thread.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
@addaleax addaleax added semver-major PRs that contain breaking changes and should be released in the next major version. and removed dont-land-on-v12.x labels Jul 4, 2021
ronag
ronag commented Jul 5, 2021
View reviewed changes
Comment thread lib/internal/bootstrap/switches/is_main_thread.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
@ronag
Update lib/internal/bootstrap/switches/is_main_thread.js
4e96f9f
@ronag

ronag commented Jul 5, 2021

Copy link
Copy Markdown
Member Author

@addaleax this is a partial revert. Isn't it better without the semver major label and instead don't land labels or what's the thought?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
addaleax
addaleax reviewed Jul 5, 2021
View reviewed changes

@addaleax addaleax left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

@ronag Yeah, I don’t see any issues with setting the HWM for the writable side here.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread lib/tty.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
@ronag
fixup: readableHighWaterMark
8283670
@ronag
ronag requested review from addaleax and jasnell July 5, 2021 19:55
@nodejs-github-bot

nodejs-github-bot commented Jul 5, 2021

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/38866/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
addaleax
addaleax approved these changes Jul 6, 2021
View reviewed changes

@addaleax addaleax left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

👍

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
benjamingr
benjamingr approved these changes Jul 6, 2021
View reviewed changes

@benjamingr benjamingr left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

good catch

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@ronag ronag added author ready PRs with CI started, the required approvals, and no outstanding review comments. dont-land-on-v12.x and removed semver-major PRs that contain breaking changes and should be released in the next major version. labels Jul 6, 2021
ronag added a commit that referenced this pull request Jul 7, 2021
@ronag
tty: enable buffering
a8a8638 
HWM was set to 0 which would cause e.g. stdout.write(...) to
always return false.

Refs: #39246

PR-URL: #39253
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
@ronag

ronag commented Jul 7, 2021

Copy link
Copy Markdown
Member Author

Landed in a8a8638
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ronag ronag closed this Jul 7, 2021
@BethGriggs BethGriggs mentioned this pull request Oct 14, 2021
2021-10-19, Version 17.0.0 (Current) #40119
Merged
2 tasks done
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@addaleax addaleax addaleax approved these changes
@benjamingr benjamingr benjamingr approved these changes
@targos targos Awaiting requested review from targos
@jasnell jasnell Awaiting requested review from jasnell

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

author ready PRs with CI started, the required approvals, and no outstanding review comments. review wanted PRs that need review. stream Issues and PRs related to Node.js streams.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

5 participants

@ronag @wa-Nadoo @nodejs-github-bot @addaleax @benjamingr
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.