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 — repl: deprecate duplicated repl features by BridgeAR · Pull Request #33294 · nodejs/node

Conversation

@BridgeAR

@BridgeAR BridgeAR commented May 7, 2020 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member
repl: deprecate repl.inputStream and repl.outputStream

The stream is exposed twice. As such it's best to rely upon the
.input and .output properties set by readline.

repl: deprecate repl._builtinLibs

This is a manually edited and outdated list of builtin modules.
Instead, it is better to rely upon the officially documented way
to get a list of builtin modules.

repl: remove obsolete completer variable

It is also assigned in readline. There is not need to assign the
variable twice.

repl: rewrite exports to module.exports

This makes sure all exports are in one place. Thus, it is easier to
see what parts are actually exported and which are not.

@nodejs/repl @nodejs/tsc PTAL

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

Sorry, something went wrong.

Uh oh!

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

bricss reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@BridgeAR BridgeAR added the semver-major PRs that contain breaking changes and should be released in the next major version. label May 7, 2020
@nodejs-github-bot nodejs-github-bot added the repl Issues and PRs related to the REPL subsystem. label May 7, 2020
@nodejs-github-bot

nodejs-github-bot commented May 7, 2020 •
edited by BridgeAR
Loading

Uh oh!

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

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/31227/
CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/2374/

All reactions

Sorry, something went wrong.

Uh oh!

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

addaleax
addaleax requested changes May 7, 2020
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.

Our deprecation policy is – luckily – quite explicit now about the fact that aliases should be preferred to runtime deprecations.

As for _builtinLibs, the mismatches with builtinModules are intentional (on my end – can’t speak for you or others, obviously), partly in order to sort out the halfway public underscored modules like _http_outgoing, _tls_common, etc.

Sorry, something went wrong.

Uh oh!

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

All reactions
@addaleax

addaleax commented May 7, 2020

Copy link
Copy Markdown
Member

Please have a look at the individual commit messages for details.

By the way:

$ git config --global alias.desc '!f() { git log --pretty="format:##### %s%n%n%b" upstream/master...HEAD --reverse; }; f'
$ git desc

Gives you a “nice” markdown printout of the commits on the current branch. I find it quite helpful when putting together PR descriptions for easier review.

jasnell and BridgeAR reacted with thumbs up emoji
All reactions
  • 👍 2 reactions

Sorry, something went wrong.

Uh oh!

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

jasnell
jasnell reviewed May 7, 2020
View reviewed changes
Comment thread doc/api/deprecations.md Resolved

Uh oh!

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

jasnell
jasnell reviewed May 7, 2020
View reviewed changes
Comment thread doc/api/deprecations.md Resolved

Uh oh!

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

@BridgeAR
BridgeAR force-pushed the deprecate-repl-features branch 2 times, most recently from c58cd3a to f098ccd Compare May 8, 2020 00:07
@BridgeAR

BridgeAR commented May 8, 2020

Copy link
Copy Markdown
Member Author

@addaleax thanks for the nice shortcut 👍

I just updated the deprecation to a doc-only deprecation which is allowed for aliases.

The builtinLibs are only exposed in the REPL and they are not up to date. They lack a few modules that should be listed. I think we should also update builtinModules to only list "actual" modules and not the underscored ones.

All reactions

Sorry, something went wrong.

Uh oh!

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

@addaleax

addaleax commented May 8, 2020

Copy link
Copy Markdown
Member

@BridgeAR The two module lists have very different purposes:

  • repl._builtinLibs gives the list of modules that should be easily accessible in the REPL.
  • module.builtinModules gives the list of modules for which require(moduleName) will load a Node.js core module instead of one from the disk.

I think they’re fundamentally irreconceilable.

All reactions

Sorry, something went wrong.

Uh oh!

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

@BridgeAR

BridgeAR commented May 8, 2020

Copy link
Copy Markdown
Member Author

If we keep _builtinLibs we should document it and name it builtinLibs. And builtinLibs should always rely upon builtinModules, otherwise we have an outdated list as we currently have (multiple modules are missing). It is possible to filter the underscores etc. that's actually what I do in #33282.

addaleax 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.

@addaleax

addaleax commented May 8, 2020

Copy link
Copy Markdown
Member

@BridgeAR That sounds like a good approach to me too 👍

All reactions

Sorry, something went wrong.

Uh oh!

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

@BridgeAR

BridgeAR commented May 8, 2020

Copy link
Copy Markdown
Member Author

@addaleax I would still deprecate _builtinLibs though. I am going to open a separate PR to add builtinLibs.

All reactions

Sorry, something went wrong.

Uh oh!

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

@BridgeAR

BridgeAR commented May 8, 2020

Copy link
Copy Markdown
Member Author

@addaleax I just opened the other PR to document the property properly. Since I changed the deprecation to a doc-deprecation, is there still something that you disagree with?

All reactions

Sorry, something went wrong.

Uh oh!

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

@BridgeAR

BridgeAR commented May 11, 2020

Copy link
Copy Markdown
Member Author

Ping @addaleax

All reactions

Sorry, something went wrong.

Uh oh!

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

@addaleax

addaleax commented May 11, 2020

Copy link
Copy Markdown
Member

@BridgeAR I think this still runtime-deprecates _buildinLibs? Once we have #33295 – which I’m a fan of, obviously – it could just be an alias.

All reactions

Sorry, something went wrong.

Uh oh!

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

@BridgeAR

BridgeAR commented May 11, 2020

Copy link
Copy Markdown
Member Author

@addaleax I changed it to a documentation-only deprecation as well. PTAL.

All reactions

Sorry, something went wrong.

Uh oh!

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

addaleax
addaleax approved these changes May 11, 2020
View reviewed changes
@BridgeAR BridgeAR added semver-minor PRs that contain new features and should be released in the next minor version. and removed semver-major PRs that contain breaking changes and should be released in the next major version. labels May 11, 2020
@BridgeAR

BridgeAR commented May 11, 2020 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member Author

I just updated the semverness to semver-minor, nice our deprecation documentation explicitly just mentions runtime and end-of-life deprecations being semver-major.

[Documentation-only] deprecations [...] are not breaking changes (semver-major)

All reactions

Sorry, something went wrong.

Uh oh!

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

@BridgeAR BridgeAR added notable-change PRs with changes that should be highlighted in changelogs. author ready PRs with CI started, the required approvals, and no outstanding review comments. labels May 11, 2020
@nodejs-github-bot

This comment has been minimized.

Sign in to view
targos
targos reviewed May 11, 2020
View reviewed changes
Comment thread doc/api/deprecations.md Resolved

Uh oh!

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

targos
targos reviewed May 11, 2020
View reviewed changes
Comment thread doc/api/deprecations.md Resolved

Uh oh!

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

@nodejs-github-bot

This comment has been minimized.

Sign in to view
BridgeAR added 2 commits May 15, 2020 02:31
@BridgeAR
repl: deprecate repl.inputStream and repl.outputStream
b32f1ae
The stream is exposed twice. As such it's best to rely upon the
.input and .output properties set by readline.

Signed-off-by: Ruben Bridgewater <[email protected]>
@BridgeAR
repl: remove obsolete completer variable
bae3a62
It is also assigned in readline. There is not need to assign the
variable twice.

Signed-off-by: Ruben Bridgewater <[email protected]>
jasnell
jasnell approved these changes May 15, 2020
View reviewed changes
@targos

targos commented May 16, 2020

Copy link
Copy Markdown
Member

Landed in 2b50cd7...ab7d520

All reactions

Sorry, something went wrong.

Uh oh!

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

@targos targos closed this May 16, 2020
targos pushed a commit that referenced this pull request May 16, 2020
@BridgeAR @targos
repl: deprecate repl.inputStream and repl.outputStream
e11b5d3
The stream is exposed twice. As such it's best to rely upon the
.input and .output properties set by readline.

Signed-off-by: Ruben Bridgewater <[email protected]>

PR-URL: #33294
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: James M Snell <[email protected]>
targos pushed a commit that referenced this pull request May 16, 2020
@BridgeAR @targos
repl: remove obsolete completer variable
ee176f1
It is also assigned in readline. There is not need to assign the
variable twice.

Signed-off-by: Ruben Bridgewater <[email protected]>

PR-URL: #33294
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: James M Snell <[email protected]>
targos pushed a commit that referenced this pull request May 16, 2020
@BridgeAR @targos
repl: deprecate repl._builtinLibs
ab7d520
This is a manually edited and outdated list of builtin modules.
Instead, it is better to rely upon the officially documented way
to get a list of builtin modules.

As a side by fix this makes sure all exports are in one place. Thus,
it is easier to see what parts are actually exported and which are
not.

Signed-off-by: Ruben Bridgewater <[email protected]>

PR-URL: #33294
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@aduh95

aduh95 commented May 16, 2020

Copy link
Copy Markdown
Contributor

@targos This has landed on master without changing the DEPXXXX to the actual ones (should be DEP0141 and DEP0142 I think):

node/doc/api/deprecations.md

Line 2671 in 48b1595

<a id="DEP0XXX"></a>

node/doc/api/deprecations.md

Line 2685 in 48b1595

<a id="DEP0XX1"></a>

All reactions

Sorry, something went wrong.

Uh oh!

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

aduh95 added a commit to aduh95/node that referenced this pull request May 16, 2020
@aduh95
repl: update deprecation codes
ccce72f
Refs: nodejs#33294
@aduh95 aduh95 mentioned this pull request May 16, 2020
repl: update deprecation codes #33430
Closed
4 tasks done
@addaleax addaleax added dont-land-on-v10.x labels May 16, 2020
codebytere pushed a commit that referenced this pull request May 16, 2020
@BridgeAR @codebytere
repl: deprecate repl.inputStream and repl.outputStream
db7bb94
The stream is exposed twice. As such it's best to rely upon the
.input and .output properties set by readline.

Signed-off-by: Ruben Bridgewater <[email protected]>

PR-URL: #33294
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: James M Snell <[email protected]>
codebytere pushed a commit that referenced this pull request May 16, 2020
@BridgeAR @codebytere
repl: remove obsolete completer variable
ed83202
It is also assigned in readline. There is not need to assign the
variable twice.

Signed-off-by: Ruben Bridgewater <[email protected]>

PR-URL: #33294
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: James M Snell <[email protected]>
codebytere pushed a commit that referenced this pull request May 16, 2020
@BridgeAR @codebytere
repl: deprecate repl._builtinLibs
7aa581f
This is a manually edited and outdated list of builtin modules.
Instead, it is better to rely upon the officially documented way
to get a list of builtin modules.

As a side by fix this makes sure all exports are in one place. Thus,
it is easier to see what parts are actually exported and which are
not.

Signed-off-by: Ruben Bridgewater <[email protected]>

PR-URL: #33294
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: James M Snell <[email protected]>
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request May 18, 2020
@aduh95 @BridgeAR
repl: update deprecation codes
a12a2d8
Refs: nodejs#33294

PR-URL: nodejs#33430
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
codebytere added a commit that referenced this pull request May 18, 2020
@codebytere
2020-05-19, Version 14.3.0 (Current)
2324d42
Notable changes:

async_hooks:
  * (SEMVER-MINOR) move PromiseHook handler to JS (Stephen Belanger) #32891
cli:
  * (SEMVER-MINOR) add `--trace-atomics-wait` flag (Anna Henningsen) #33292
fs:
  * (SEMVER-MINOR) add .ref() and .unref() methods to watcher classes (rickyes) #33134
http:
  * (SEMVER-MINOR) expose http.validate-header-name/value (osher) #33119
repl:
  * (SEMVER-MINOR) deprecate repl._builtinLibs (Ruben Bridgewater) #33294
  * (SEMVER-MINOR) remove obsolete completer variable (Ruben Bridgewater) #33294
  * (SEMVER-MINOR) deprecate repl.inputStream and repl.outputStream (Ruben Bridgewater) #33294
  * (SEMVER-MINOR) improve repl autocompletion for require calls (Ruben Bridgewater) #33282
  * (SEMVER-MINOR) replace hard coded core module list with actual list (Ruben Bridgewater) #33282
  * (SEMVER-MINOR) show reference errors during preview (Ruben Bridgewater) #33282
  * (SEMVER-MINOR) improve repl preview (Ruben Bridgewater) #33282
src:
  * add support for TLA (Gus Caplan) #30370
test:
  * (SEMVER-MINOR) refactor test/parallel/test-bootstrap-modules.js (Ruben Bridgewater) #33282

PR-URL: TODO
@codebytere codebytere mentioned this pull request May 18, 2020
v14.3.0 proposal #33452
Merged
codebytere added a commit that referenced this pull request May 19, 2020
@codebytere
2020-05-19, Version 14.3.0 (Current)
76a52dd
Notable changes:

async_hooks:
  * (SEMVER-MINOR) move PromiseHook handler to JS (Stephen Belanger) #32891
cli:
  * (SEMVER-MINOR) add `--trace-atomics-wait` flag (Anna Henningsen) #33292
fs:
  * (SEMVER-MINOR) add .ref() and .unref() methods to watcher classes (rickyes) #33134
http:
  * (SEMVER-MINOR) expose http.validate-header-name/value (osher) #33119
repl:
  * (SEMVER-MINOR) deprecate repl._builtinLibs (Ruben Bridgewater) #33294
  * (SEMVER-MINOR) deprecate repl.inputStream and repl.outputStream (Ruben Bridgewater) #33294
  * (SEMVER-MINOR) show reference errors during preview (Ruben Bridgewater) #33282
  * (SEMVER-MINOR) improve repl preview (Ruben Bridgewater) #33282
src:
  * add support for TLA (Gus Caplan) #30370

PR-URL: TODO
codebytere added a commit that referenced this pull request May 19, 2020
@codebytere
2020-05-19, Version 14.3.0 (Current)
a33e87c
Notable changes:

async_hooks:
  * (SEMVER-MINOR) move PromiseHook handler to JS (Stephen Belanger) #32891
cli:
  * (SEMVER-MINOR) add `--trace-atomics-wait` flag (Anna Henningsen) #33292
fs:
  * (SEMVER-MINOR) add .ref() and .unref() methods to watcher classes (rickyes) #33134
http:
  * (SEMVER-MINOR) expose http.validate-header-name/value (osher) #33119
repl:
  * (SEMVER-MINOR) deprecate repl._builtinLibs (Ruben Bridgewater) #33294
  * (SEMVER-MINOR) deprecate repl.inputStream and repl.outputStream (Ruben Bridgewater) #33294
  * (SEMVER-MINOR) show reference errors during preview (Ruben Bridgewater) #33282
  * (SEMVER-MINOR) improve repl preview (Ruben Bridgewater) #33282
src:
  * add support for TLA (Gus Caplan) #30370

PR-URL: TODO
codebytere added a commit that referenced this pull request May 19, 2020
@codebytere
2020-05-19, Version 14.3.0 (Current)
5d81e4d
Notable changes:

async_hooks:
  * (SEMVER-MINOR) move PromiseHook handler to JS (Stephen Belanger) #32891
cli:
  * (SEMVER-MINOR) add `--trace-atomics-wait` flag (Anna Henningsen) #33292
fs:
  * (SEMVER-MINOR) add .ref() and .unref() methods to watcher classes (rickyes) #33134
http:
  * (SEMVER-MINOR) expose http.validate-header-name/value (osher) #33119
repl:
  * (SEMVER-MINOR) deprecate repl._builtinLibs (Ruben Bridgewater) #33294
  * (SEMVER-MINOR) deprecate repl.inputStream and repl.outputStream (Ruben Bridgewater) #33294
  * (SEMVER-MINOR) show reference errors during preview (Ruben Bridgewater) #33282
  * (SEMVER-MINOR) improve repl preview (Ruben Bridgewater) #33282
src:
  * add support for TLA (Gus Caplan) #30370

PR-URL: TODO
codebytere added a commit that referenced this pull request May 19, 2020
@codebytere
2020-05-19, Version 14.3.0 (Current)
c841f51
Notable changes:

async_hooks:
  * (SEMVER-MINOR) move PromiseHook handler to JS (Stephen Belanger) #32891
cli:
  * (SEMVER-MINOR) add `--trace-atomics-wait` flag (Anna Henningsen) #33292
fs:
  * (SEMVER-MINOR) add .ref() and .unref() methods to watcher classes (rickyes) #33134
http:
  * (SEMVER-MINOR) expose http.validate-header-name/value (osher) #33119
repl:
  * (SEMVER-MINOR) deprecate repl._builtinLibs (Ruben Bridgewater) #33294
  * (SEMVER-MINOR) deprecate repl.inputStream and repl.outputStream (Ruben Bridgewater) #33294
  * (SEMVER-MINOR) show reference errors during preview (Ruben Bridgewater) #33282
  * (SEMVER-MINOR) improve repl preview (Ruben Bridgewater) #33282
src:
  * add support for TLA (Gus Caplan) #30370

PR-URL: #33452
@codebytere codebytere mentioned this pull request May 20, 2020
feat: handle unmarked DEP0XXX tags during landing and release nodejs/node-core-utils#420
Merged
@aduh95 aduh95 mentioned this pull request Sep 5, 2024
repl: runtime deprecate inputStream, outputStream and _builtinLibs #54750
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnell jasnell jasnell approved these changes
@addaleax addaleax addaleax approved these changes
+1 more reviewer
@targos targos targos approved these changes
Reviewers whose approvals may not affect merge requirements

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. notable-change PRs with changes that should be highlighted in changelogs. repl Issues and PRs related to the REPL subsystem. semver-minor PRs that contain new features and should be released in the next minor version.

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.

6 participants

@BridgeAR @nodejs-github-bot @addaleax @targos @aduh95 @jasnell
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.