Skip to content

doc: clarify examples section in REPL doc #57762

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

Merged

Conversation

dario-piotrowicz
Copy link
Member

@dario-piotrowicz dario-piotrowicz commented Apr 5, 2025

This is pretty minor, but I just figured a little bit of polishing wouldn't hurt 🙂

The current examples presented at the bottom of the REPL doc have three issues which I am trying to address here:

  • they look like they're part of the section above (on how to run multiple REPL instances in the same process) but they are not

    see the table of contents at the top of the REPL page:

    before after
    before-1 (the examples are under the multiple REPL section) after-1 (the examples are listed on their own)
  • they link to old gists with code that uses outdates JS syntax (e.g var) and deprecated utilities (e.g. new Buffer)
    (so not something really ideal to present as an official example)

  • the alert informing readers not to use the second example in production environments can be wrongly interpreted as to refer to both examples

    (this might be just me, but when I read the text there it really looked to me like the warning was referring to both examples and that This example was likely a typo and that These examples was supposed to be used)

    before after
    before the warning is ananbiguously in the section specific for the example

@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to the documentations. repl Issues and PRs related to the REPL subsystem. labels Apr 5, 2025
@dario-piotrowicz dario-piotrowicz force-pushed the dario/doc/repl-examples-format branch from bb2fea9 to 1edd98a Compare April 5, 2025 18:33
doc/api/repl.md Outdated

* running a "full-featured" (`terminal`) REPL over
a `net.Server` and `net.Socket` instance:
<https://gist.github.com/TooTallNate/2209310>.
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if it still makes sense to link to an external gist, maybe we should copy the code over (not sure how it's licensed though it seems short enough to not fall under copyrightable code).

Copy link
Member Author

Choose a reason for hiding this comment

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

I totally agree, an external gist to a personal github user feels off to me 😕

Copy link
Member

Choose a reason for hiding this comment

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

IMO, We should create our own example derived from the sample and still link over to the gist for credit.

Copy link
Member Author

Choose a reason for hiding this comment

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

I can definitely do that 🙂👍

Copy link
Member Author

Choose a reason for hiding this comment

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

done 🙂 0c1abbe

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah yeah, one last question! 🙂

Do you think that the multi REPL section should also be moved under the new examples section? 🙂

dario-piotrowicz and others added 3 commits April 6, 2025 20:10
The current examples presented at the bottom
of the REPL doc have two issues:
 - they look like they're part of the section
   above (on how to run multiple REPL instances
   in the same process) but they are not
 - the alert informing readers not to use the
   second example in production environments
   can be wrongly interpreted as to refer to
   both examples
The changes here address both these issues
Capitalize list items

Co-authored-by: Antoine du Hamel <[email protected]>
@dario-piotrowicz dario-piotrowicz force-pushed the dario/doc/repl-examples-format branch from 29b0720 to 0c1abbe Compare April 6, 2025 20:25
doc/api/repl.md Outdated
Comment on lines 1095 to 1100
// Hack to thread stdin and stdout
// simultaneously in curl's single thread
const iv = setInterval(() => {
res.write(buf0);
}, 100);

Copy link
Contributor

Choose a reason for hiding this comment

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

Is this still needed? I can't see what difference it makes

Copy link
Member Author

@dario-piotrowicz dario-piotrowicz Apr 6, 2025

Choose a reason for hiding this comment

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

No I just tried the example without it (I'm on Ubuntu) and it does not seem necessary

I wasn't actually fully sure why this was needed and feared that removing might break the example under some conditions / on some platform? 🤷

However do think that quite likely this is not needed, so I'm totally happy removing it, we can always re-add it if in some cases it is right? 🙂

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed 🙂 a977fbe

(PS: thanks so much for the suggestion 🫶)

@aduh95 aduh95 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Apr 8, 2025
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 8, 2025
@nodejs-github-bot nodejs-github-bot merged commit b396991 into nodejs:main Apr 8, 2025
26 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in b396991

@dario-piotrowicz dario-piotrowicz deleted the dario/doc/repl-examples-format branch April 8, 2025 23:01
JonasBa pushed a commit to JonasBa/node that referenced this pull request Apr 11, 2025
The current examples presented at the bottom
of the REPL doc have two issues:
 - they look like they're part of the section
   above (on how to run multiple REPL instances
   in the same process) but they are not
 - the alert informing readers not to use the
   second example in production environments
   can be wrongly interpreted as to refer to
   both examples
The changes here address both these issues

PR-URL: nodejs#57762
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
RafaelGSS pushed a commit that referenced this pull request May 1, 2025
The current examples presented at the bottom
of the REPL doc have two issues:
 - they look like they're part of the section
   above (on how to run multiple REPL instances
   in the same process) but they are not
 - the alert informing readers not to use the
   second example in production environments
   can be wrongly interpreted as to refer to
   both examples
The changes here address both these issues

PR-URL: #57762
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
RafaelGSS pushed a commit that referenced this pull request May 2, 2025
The current examples presented at the bottom
of the REPL doc have two issues:
 - they look like they're part of the section
   above (on how to run multiple REPL instances
   in the same process) but they are not
 - the alert informing readers not to use the
   second example in production environments
   can be wrongly interpreted as to refer to
   both examples
The changes here address both these issues

PR-URL: #57762
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
aduh95 pushed a commit that referenced this pull request May 6, 2025
The current examples presented at the bottom
of the REPL doc have two issues:
 - they look like they're part of the section
   above (on how to run multiple REPL instances
   in the same process) but they are not
 - the alert informing readers not to use the
   second example in production environments
   can be wrongly interpreted as to refer to
   both examples
The changes here address both these issues

PR-URL: #57762
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
RafaelGSS pushed a commit that referenced this pull request May 14, 2025
The current examples presented at the bottom
of the REPL doc have two issues:
 - they look like they're part of the section
   above (on how to run multiple REPL instances
   in the same process) but they are not
 - the alert informing readers not to use the
   second example in production environments
   can be wrongly interpreted as to refer to
   both examples
The changes here address both these issues

PR-URL: #57762
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
aduh95 pushed a commit that referenced this pull request May 16, 2025
The current examples presented at the bottom
of the REPL doc have two issues:
 - they look like they're part of the section
   above (on how to run multiple REPL instances
   in the same process) but they are not
 - the alert informing readers not to use the
   second example in production environments
   can be wrongly interpreted as to refer to
   both examples
The changes here address both these issues

PR-URL: #57762
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
aduh95 pushed a commit that referenced this pull request May 17, 2025
The current examples presented at the bottom
of the REPL doc have two issues:
 - they look like they're part of the section
   above (on how to run multiple REPL instances
   in the same process) but they are not
 - the alert informing readers not to use the
   second example in production environments
   can be wrongly interpreted as to refer to
   both examples
The changes here address both these issues

PR-URL: #57762
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
aduh95 pushed a commit that referenced this pull request May 17, 2025
The current examples presented at the bottom
of the REPL doc have two issues:
 - they look like they're part of the section
   above (on how to run multiple REPL instances
   in the same process) but they are not
 - the alert informing readers not to use the
   second example in production environments
   can be wrongly interpreted as to refer to
   both examples
The changes here address both these issues

PR-URL: #57762
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
aduh95 pushed a commit that referenced this pull request May 17, 2025
The current examples presented at the bottom
of the REPL doc have two issues:
 - they look like they're part of the section
   above (on how to run multiple REPL instances
   in the same process) but they are not
 - the alert informing readers not to use the
   second example in production environments
   can be wrongly interpreted as to refer to
   both examples
The changes here address both these issues

PR-URL: #57762
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
aduh95 pushed a commit that referenced this pull request May 19, 2025
The current examples presented at the bottom
of the REPL doc have two issues:
 - they look like they're part of the section
   above (on how to run multiple REPL instances
   in the same process) but they are not
 - the alert informing readers not to use the
   second example in production environments
   can be wrongly interpreted as to refer to
   both examples
The changes here address both these issues

PR-URL: #57762
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. doc Issues and PRs related to the documentations. repl Issues and PRs related to the REPL subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants