[node/readline] Expose 'line' & 'cursor' properties #40513
Conversation
typescript-bot
added
Popular package
|
@Js-Brecht Thank you for submitting this PR! 🔔 @microsoft @DefinitelyTyped @jkomyno @a-tarasyuk @alvis @r3nya @btoueg @BrunoScheufler @smac89 @tellnes @Touffy @DeividasBakanas @eyqs @Flarna @Hannes-Magnusson-CK @KSXGitHub @hoo29 @kjin @ajafff @islishude @mwiktorczyk @mohsen1 @n-e @octo-sniffle @galkin @parambirs @eps1lon @SimonSchick @ThomasdenH @WilcoBakker @wwwy3y3 @ZaneHannanAU @samuela @kuehlein @j-oliveras @bhongy @chyzwar @trivikr @nguymin4 @yoursunny @qwelias @jeremiergz - please review this PR in the next few days. Be sure to explicitly select If no reviewer appears after a week, a DefinitelyTyped maintainer will review the PR instead. |
typescript-bot
moved this from Waiting for Reviewers
to Needs Author Attention
in Pull Request Status Board
typescript-bot
added
The Travis CI build failed
and removed
Awaiting reviewer feedback
labels
|
@Js-Brecht The Travis CI build failed! Please review the logs for more information. Once you've pushed the fixes, the build will automatically re-run. Thanks! |
|
It's usually not needed to updated typings for EOL versions of node (9, 11). |
typescript-bot
added
the
Owner Approved
|
@Flarna Is it EOL for 9-11 already? I just figured that since they are valid properties for those versions as well, that they should be typed. Or are you saying they are EOL for Anybody have an idea why the It was successful at one point. I recall it failed after I originally submitted the PR, then I added the v11 test, and it was successful when it ran again. I pushed the v9 change, and it failed again, the same way as the first time. These changes are so innocuous, I don't know what is breaking it. |
|
@Js-Brecht We avoid updating EOL versions as the node team itself doesn't support them either, 10 is LTS (odd vs. even). |
|
@SimonSchick Ahh, I see. I should have looked at https://nodejs.org/en/about/releases/. I will revert the changes to v9 and v11 |
typescript-bot
moved this from Needs Author Attention
to Check and Merge
in Pull Request Status Board
|
A definition owner has approved this PR ⭐️. A maintainer will merge this PR shortly. If it shouldn't be merged yet, please leave a comment saying so and we'll wait. Thank you for your contribution to DefinitelyTyped! |
|
@Js-Brecht so I can see these properties currently exist but are not documented. |
|
@SimonSchick Sure. The plan was to expose I could ask for them to explicitly approve making these two properties public.
|
|
Or if you'd like to wait until the documentation PR is approved, I'd be okay with that |
typescript-bot
moved this from Check and Merge
to Needs Author Attention
in Pull Request Status Board
|
@Js-Brecht The Travis CI build failed! Please review the logs for more information. Once you've pushed the fixes, the build will automatically re-run. Thanks! |
Documents the existence and purpose of the `line` and `cursor` properties. `line` can be used for reading the current input value during runtime, if reading from a TTY stream. Both properties are necessary when developing a custom CLI input process using `readline` as a backend. Refs: nodejs#30347 Refs: DefinitelyTyped/DefinitelyTyped#40513
|
@Js-Brecht I haven't seen anything from you in a while and this PR currently has problems that prevent it from being merged. The PR will be closed tomorrow if there aren't new commits to fix the issues. |
|
Finally had a moment to file my PR with nodejs. See nodejs/node#30667. @SimonSchick Is it possible to get @typescript-bot to hold off until that PR gets reviewed and accepted/rejected? |
|
@amcasey is it possible to reopen this PR, or do I need to open a new one? nodejs/node#30667 landed here nodejs/node@bd9be04 See docs here: |
Documents the existence and purpose of the `line` and `cursor` properties. `line` can be used for reading the current input value during runtime, if reading from a TTY stream. Both properties are necessary when developing a custom CLI input process using `readline` as a backend. Refs: #30347 Refs: DefinitelyTyped/DefinitelyTyped#40513 PR-URL: #30667 Reviewed-By: Anna Henningsen <anna@addaleax.net>
|
@Js-Brecht Reopened. Good to go? |
|
Yes, it is good to go |
|
I don't know what's up with the bot, but I'll merge it once CI completes. |
typescript-bot
added
Other Approved
typescript-bot
moved this from Check and Merge
to Needs Author Attention
in Pull Request Status Board
typescript-bot
added
The Travis CI build failed
Abandoned
|
Note to future readers - the Travis build passed on re-run. |
|
I just published |
|
I just published |
Documents the existence and purpose of the `line` and `cursor` properties. `line` can be used for reading the current input value during runtime, if reading from a TTY stream. Both properties are necessary when developing a custom CLI input process using `readline` as a backend. Refs: #30347 Refs: DefinitelyTyped/DefinitelyTyped#40513 PR-URL: #30667 Reviewed-By: Anna Henningsen <anna@addaleax.net>
Documents the existence and purpose of the `line` and `cursor` properties. `line` can be used for reading the current input value during runtime, if reading from a TTY stream. Both properties are necessary when developing a custom CLI input process using `readline` as a backend. Refs: #30347 Refs: DefinitelyTyped/DefinitelyTyped#40513 PR-URL: #30667 Reviewed-By: Anna Henningsen <anna@addaleax.net>
The current "line" and "cursor" states are often necessary when developing applications that use readline for prompts.
While the properties should be accessible, they are also meant to be used internally, and modifying them could result in unexpected behavior when drawing, which is why they are
readonly. They can be modified safely, butcursorwould have to be modified in tandem withline. IMO, it's better to make themreadonly, so overwriting them will be an explicit effort; that way whoever does it is fully aware of what they are doing.See nodejs/node#30347
Please fill in this template.
npm test.)npm run lint package-name(ortscif notslint.jsonis present).If changing an existing definition:
tslint.jsoncontaining{ "extends": "dtslint/dt.json" }. If for reason the any rule need to be disabled, disable it for that line using// tslint:disable-next-line [ruleName]and not for whole package so that the need for disabling can be reviewed.