Skip to content

Conversation

@BridgeAR
Copy link
Member

The documentation currently states that it's not intended to directly
instantiate REPL instances. It is unknown for what reason that's
recommended as it does seem a proper way to handle new REPL instances.

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

The documentation currently states that it's not intended to directly instantiate REPL instances. It is unknown for what reason that's recommended as it does seem a proper way to handle new REPL instances.
@nodejs-github-botnodejs-github-bot added doc Issues and PRs related to the documentations. repl Issues and PRs related to the REPL subsystem. labels Dec 13, 2019
@Trott
Copy link
Member

Bottom reference needs to be corrected but other than that, looks good to me.

@BridgeAR
Copy link
MemberAuthor

@BridgeARBridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 13, 2019
@addaleax
Copy link
Member

@BridgeAR Using repl.start() and new REPLServer() are not equivalent, though, and the documentation seems to imply otherwise? Can you also change the code to work the same in both cases, and then add that to the YAML changelog?

@addaleaxaddaleax removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 14, 2019
@BridgeAR
Copy link
MemberAuthor

@addaleax I'll add a note about the behavior difference. The behavior of .start is somewhat special though: it just adds the very first repl instance as property on the export. I am not certain what reason there was to add this. The commit that adds this property does not indicate any reasons for it either (the description and main code change is about a different feature).
What do you think about checking the usage with Gzemnid and citgm and to deprecate that property if it's not frequently used? My guess is that this property is not used at all.

@addaleax
Copy link
Member

@BridgeAR It adds the repl to replMap. That’s the more important bit here imo.

What do you think about checking the usage with Gzemnid and citgm and to deprecate that property if it's not frequently used? My guess is that this property is not used at all.

I think both tools would be useless in this case, and I see no reason to deprecate. repl.repl is the way to access the current REPL instance from the REPL itself, and I expect programmatic usage to be minimal.

@BridgeAR
Copy link
MemberAuthor

@addaleax I somehow did not really think about accessing the current REPL. That is indeed something we should not remove. I think we should move that into the constructor and either only make this accessible in the standalone repl or each instance instead of exporting the very first repl instance that is created with .start() in each repl instance.
I'll mark this PR as blocked and open a PR to implement the exposure when using it as standalone REPL.

It adds the repl to replMap

I removed replMap in a recent commit. So that's not an issue anymore.

@BridgeARBridgeAR added the blocked PRs that are blocked by other issues or PRs. label Dec 15, 2019
@BridgeARBridgeAR mentioned this pull request Dec 15, 2019
4 tasks
@BridgeARBridgeAR removed the blocked PRs that are blocked by other issues or PRs. label Jan 3, 2020
BridgeAR added a commit that referenced this pull request Jan 3, 2020
The documentation currently states that it's not intended to directly instantiate REPL instances. It is unknown for what reason that's recommended as it does seem a proper way to handle new REPL instances. PR-URL: #30928 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Yongsheng Zhang <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: James M Snell <[email protected]>
@BridgeAR
Copy link
MemberAuthor

Landed in 84b15eb 🎉

@BridgeARBridgeAR closed this Jan 3, 2020
targos pushed a commit that referenced this pull request Jan 6, 2020
The documentation currently states that it's not intended to directly instantiate REPL instances. It is unknown for what reason that's recommended as it does seem a proper way to handle new REPL instances. PR-URL: #30928 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Yongsheng Zhang <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: James M Snell <[email protected]>
@BridgeARBridgeAR mentioned this pull request Jan 7, 2020
targos pushed a commit that referenced this pull request Jan 14, 2020
The documentation currently states that it's not intended to directly instantiate REPL instances. It is unknown for what reason that's recommended as it does seem a proper way to handle new REPL instances. PR-URL: #30928 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Yongsheng Zhang <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: James M Snell <[email protected]>
@targostargos mentioned this pull request Jan 15, 2020
@BridgeARBridgeAR deleted the update-repl-start-doc branch January 20, 2020 12:07
BethGriggs pushed a commit that referenced this pull request Feb 6, 2020
The documentation currently states that it's not intended to directly instantiate REPL instances. It is unknown for what reason that's recommended as it does seem a proper way to handle new REPL instances. PR-URL: #30928 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Yongsheng Zhang <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: James M Snell <[email protected]>
@MylesBorinsMylesBorins mentioned this pull request Feb 8, 2020
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

docIssues and PRs related to the documentations.replIssues and PRs related to the REPL subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants

@BridgeAR@Trott@addaleax@jasnell@targos@ZYSzys@nodejs-github-bot