Skip to content

repl: extract and standardize history from both repl and interface #58225

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

puskin
Copy link
Contributor

@puskin puskin commented May 8, 2025

Refactored and decoupled the history logic into its own separate module and made both REPL and readline depend on it as little as possible, still not breaking functionality.

Updated the repl.setupHistory function to accept more parameters than just a string for the history file path and a callback. Before this change you had to manually update the REPL instance to accept history parameters. With the decoupling of this PR, you can now achieve the same result calling the updated method:

repl.setupHistory({
    historyFile: './path/to/history/file',
    historySize: 50,
    removeHistoryDuplicates: true,
    onHistoryFileLoaded: callback,
  });

@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 May 8, 2025
@puskin puskin force-pushed the repl-extract-history branch 2 times, most recently from 49982b2 to 63f9371 Compare May 8, 2025 07:57
Copy link

codecov bot commented May 8, 2025

Codecov Report

Attention: Patch coverage is 92.92237% with 31 lines in your changes missing coverage. Please review.

Project coverage is 90.17%. Comparing base (f75a126) to head (cf8f53f).
Report is 31 commits behind head on main.

Files with missing lines Patch % Lines
lib/internal/repl/history.js 91.34% 31 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main   #58225   +/-   ##
=======================================
  Coverage   90.17%   90.17%           
=======================================
  Files         630      629    -1     
  Lines      186756   186826   +70     
  Branches    36664    36686   +22     
=======================================
+ Hits       168399   168477   +78     
+ Misses      11179    11151   -28     
- Partials     7178     7198   +20     
Files with missing lines Coverage Δ
lib/internal/main/repl.js 94.11% <100.00%> (ø)
lib/internal/readline/interface.js 97.24% <100.00%> (-0.01%) ⬇️
lib/internal/repl.js 100.00% <100.00%> (ø)
lib/repl.js 95.01% <100.00%> (+0.02%) ⬆️
lib/internal/repl/history.js 92.03% <91.34%> (+3.57%) ⬆️

... and 47 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 marked this pull request as draft May 8, 2025 08:48
@puskin puskin force-pushed the repl-extract-history branch from 63f9371 to 01dcfe4 Compare May 8, 2025 08:55
@puskin puskin marked this pull request as ready for review May 8, 2025 08:55
@puskin puskin force-pushed the repl-extract-history branch 2 times, most recently from 2510895 to 6e36f16 Compare May 8, 2025 14:47
@puskin puskin force-pushed the repl-extract-history branch from 6e36f16 to cf8f53f Compare May 8, 2025 18:58
@puskin
Copy link
Contributor Author

puskin commented May 15, 2025

Anyone from @nodejs/repl who wants to give this another review ? 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants