Skip to content

repl: add support for multiline history #57400

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
merged 13 commits into from
Apr 13, 2025

Conversation

puskin
Copy link
Contributor

@puskin puskin commented Mar 10, 2025

Previously, when navigating the history in the Node.js REPL, multiline commands were displayed line by line, making it difficult to review and edit complete blocks of input.

This PR improves the REPL history behavior by treating multiline commands as a single entry. Now, when scrolling through history, entire blocks of multiline input are displayed together, improving usability and consistency.

Key changes:

  • Adjusted history handling to store and retrieve multiline commands as single entries.
  • Updated navigation logic to properly display multiline inputs.
  • Improved editing experience, allowing users to modify multiline history entries in place.
  • Changed the multiline indicator: from ... to |, to keep the indentation consistent
  • Added the multiline indicator also when editing the history, not only when adding new lines
  • If the last command caused some kind of error, you can now edit the multiline history, and if you remove the error, the history which caused the error will be replaced with the correct one

Important:

This change relies on the fact that I am now formatting new history entries and saving them in the .node_replhistory file in a specific format. While this does not break backward compatibility, it does mean that switching between older and newer versions (i.e., versions without and with this change) will result in multiline history entries not being preserved if they were created with the latest changes.

Impact:

This change significantly enhances the REPL experience for users who frequently enter complex or multiline expressions, making command recall and editing more intuitive.

Testing:

  • Manually tested with various multiline inputs, both on unix and windows systems
  • Verified history navigation behaves as expected with both single-line and multiline commands.

Demos

Before:

before

After:

after

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. readline Issues and PRs related to the built-in readline module. repl Issues and PRs related to the REPL subsystem. labels Mar 10, 2025
@puskin puskin force-pushed the repl-multiline-history branch 2 times, most recently from 9137def to 5f9b8f1 Compare March 12, 2025 10:27
@puskin puskin marked this pull request as ready for review March 12, 2025 10:27
Copy link

codecov bot commented Mar 12, 2025

Codecov Report

Attention: Patch coverage is 96.62921% with 6 lines in your changes missing coverage. Please review.

Project coverage is 90.24%. Comparing base (27f98c3) to head (cab0f16).
Report is 555 commits behind head on main.

Files with missing lines Patch % Lines
lib/internal/readline/interface.js 95.38% 4 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #57400      +/-   ##
==========================================
+ Coverage   90.20%   90.24%   +0.03%     
==========================================
  Files         630      630              
  Lines      185307   185613     +306     
  Branches    36269    36400     +131     
==========================================
+ Hits       167162   167507     +345     
+ Misses      11084    10994      -90     
- Partials     7061     7112      +51     
Files with missing lines Coverage Δ
lib/internal/repl/history.js 88.46% <100.00%> (+2.19%) ⬆️
lib/internal/repl/utils.js 96.79% <100.00%> (+0.52%) ⬆️
lib/repl.js 95.03% <100.00%> (+0.13%) ⬆️
lib/internal/readline/interface.js 96.76% <95.38%> (-0.19%) ⬇️

... and 145 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@puskin puskin force-pushed the repl-multiline-history branch from 5f9b8f1 to 8b261ae Compare March 12, 2025 12:26
@aduh95
Copy link
Contributor

aduh95 commented Mar 16, 2025

/cc @nodejs/repl

@puskin
Copy link
Contributor Author

puskin commented Mar 17, 2025

@BridgeAR you, being the requestor, maybe want to give this a shot 😄

Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

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

This is a great start!

I did play around with it and I found the following issues:

  1. A history item is now not properly deduplicated anymore if it spans over multiple lines. That is the case with other entries when jumping through the history.
  2. When entering a multiline command that fails to evaluate, it is handled as individual lines. Especially those cases would be great to "correct". For comfort reasons, I would even consider deleting the broken one, if that one is edited and saved successfully right as following command.
  3. Moving up and down while working on a multiline entry feels somewhat off. It is opinionated, but I believe the former history entry should only be shown in case the cursor is at the top line while editing a multiline input. The reason is, that I likely want to finish the input and not change to the former history entry.

I would also change the ... to |. That way the indentation while entering something is identical. I guess we can still look into doing this potentially while going through the entries in the history but at least it is nicer during initial entry.

@puskin puskin force-pushed the repl-multiline-history branch 5 times, most recently from a434fe4 to 8e71cdf Compare March 18, 2025 15:13
@puskin puskin force-pushed the repl-multiline-history branch 2 times, most recently from 897b341 to 3418b3b Compare March 19, 2025 21:01
@puskin puskin force-pushed the repl-multiline-history branch from 3418b3b to b41273a Compare March 21, 2025 16:28
@puskin puskin requested a review from BridgeAR March 25, 2025 15:42
Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

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

This is great work! I am looking forward to landing this change!

I left a few comments, mainly to understand some things better.

@puskin puskin force-pushed the repl-multiline-history branch from 603b1d3 to 23f7e47 Compare April 10, 2025 15:20
This will make it consistent when loading the new history format with an old node binary
Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you for the great work! 🥳

@BridgeAR BridgeAR added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 12, 2025
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 12, 2025
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Apr 12, 2025

@nodejs-github-bot
Copy link
Collaborator

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 13, 2025
@nodejs-github-bot
Copy link
Collaborator

@BridgeAR BridgeAR added 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 13, 2025
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 13, 2025
@nodejs-github-bot nodejs-github-bot merged commit 4a4aa58 into nodejs:main Apr 13, 2025
67 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 4a4aa58

@lpinca
Copy link
Member

lpinca commented Apr 14, 2025

It seems that test/parallel/test-repl-multiline-navigation is flaky. See https://ci.nodejs.org/job/node-test-commit-aix/56805/nodes=aix72-ppc64/testReport/junit/(root)/parallel/test_repl_multiline_navigation/

---
duration_ms: 324.926
exitcode: 1
severity: fail
stack: |-
  node:assert:95
    throw new AssertionError(obj);
    ^

  AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:

  2 !== 1

      at /home/iojs/build/workspace/node-test-commit-aix/nodes/aix72-ppc64/test/parallel/test-repl-multiline-navigation.js:132:12
      at /home/iojs/build/workspace/node-test-commit-aix/nodes/aix72-ppc64/test/common/index.js:400:17
      at /home/iojs/build/workspace/node-test-commit-aix/nodes/aix72-ppc64/test/common/index.js:435:15
      at REPLServer.<anonymous> (node:internal/repl/history:120:9)
      at Object.onceWrapper (node:events:621:28)
      at REPLServer.emit (node:events:507:28)
      at REPLServer.emit (node:domain:489:12)
      at onwritten (node:internal/repl/history:156:14)
      at FSReqCallback.wrapper [as oncomplete] (node:fs:803:5)
      at FSReqCallback.callbackTrampoline (node:internal/async_hooks:130:17) {
    generatedMessage: true,
    code: 'ERR_ASSERTION',
    actual: 2,
    expected: 1,
    operator: 'strictEqual'
  }

  Node.js v24.0.0-pre
...

Can you take a look?

@puskin
Copy link
Contributor Author

puskin commented Apr 15, 2025

@lpinca thanks for the report!
It should get fixed here: #57874 😄

RafaelGSS pushed a commit that referenced this pull request May 1, 2025
PR-URL: #57400
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Jordan Harband <[email protected]>
RafaelGSS pushed a commit that referenced this pull request May 2, 2025
PR-URL: #57400
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Jordan Harband <[email protected]>
aduh95 pushed a commit that referenced this pull request May 6, 2025
PR-URL: #57400
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Jordan Harband <[email protected]>
aduh95 pushed a commit that referenced this pull request May 6, 2025
PR-URL: #57400
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Jordan Harband <[email protected]>
RafaelGSS pushed a commit that referenced this pull request May 14, 2025
PR-URL: #57400
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Jordan Harband <[email protected]>
aduh95 pushed a commit that referenced this pull request May 16, 2025
PR-URL: #57400
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Jordan Harband <[email protected]>
aduh95 pushed a commit that referenced this pull request May 17, 2025
PR-URL: #57400
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Jordan Harband <[email protected]>
aduh95 pushed a commit that referenced this pull request May 17, 2025
PR-URL: #57400
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Jordan Harband <[email protected]>
aduh95 pushed a commit that referenced this pull request May 17, 2025
PR-URL: #57400
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Jordan Harband <[email protected]>
@aduh95 aduh95 added the backport-requested-v22.x PRs awaiting manual backport to the v22.x-staging branch. label May 18, 2025
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. backport-requested-v22.x PRs awaiting manual backport to the v22.x-staging branch. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. needs-ci PRs that need a full CI run. notable-change PRs with changes that should be highlighted in changelogs. readline Issues and PRs related to the built-in readline module. 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
Development

Successfully merging this pull request may close these issues.

6 participants