readline: initialize input before history manager#61538
readline: initialize input before history manager#61538Zohaibarif69 wants to merge 1 commit intonodejs:mainfrom
Conversation
nodejs-github-bot
added
needs-ci
lib/internal/readline/interface.js
Outdated
| let historySize; | ||
| let history; | ||
| let removeHistoryDuplicates; |
There was a problem hiding this comment.
hoisting these is not needed so you can keep them scoped like the original
| let historySize; | |
| let history; | |
| let removeHistoryDuplicates; |
lib/internal/readline/interface.js
Outdated
| historySize = input.historySize; | ||
| history = input.history; | ||
| removeHistoryDuplicates = input.removeHistoryDuplicates; |
There was a problem hiding this comment.
revert this
| historySize = input.historySize; | |
| history = input.history; | |
| removeHistoryDuplicates = input.removeHistoryDuplicates; | |
| const historySize = input.historySize; | |
| const history = input.history; | |
| const removeHistoryDuplicates = input.removeHistoryDuplicates; |
| this[kSubstringSearch] = null; | ||
| this.output = output; | ||
| this.input = input; | ||
| this.setupHistoryManager(input); |
There was a problem hiding this comment.
this reodering only resolve this.input being undefine but still doesn't fix accidental REPL history init where readline.createInterface(input) is not supposed to touch REPL history but setupHistoryManager(input) ends up seeing a truthy input.onHistoryFileLoaded and calls ReplHistory.initialize() anyway
| const assert = require('assert'); | ||
|
|
||
| assert.doesNotThrow(() => { | ||
| const input = new Proxy({}, { |
There was a problem hiding this comment.
you’re proxying {} (not a real stream). this test case will throw exceptions
|
@thisalihassan Thanks for the feedback! I’ll address the scoping, test, and history initialization issues and update the PR shortly. |
Zohaibarif69
force-pushed
the
fix-readline-history-init
branch
from
736d855 to
e3a00ce
Compare
|
@thisalihassan Thanks — I’ve addressed the scoping, history initialization, and test issues you mentioned. |
Zohaibarif69
force-pushed
the
fix-readline-history-init
branch
from
e3a00ce to
9dc21a9
Compare
|
@thisalihassan I’ve cleaned up the diff, reverted the unnecessary scoping changes, fixed the history initialization logic, and updated the test to use real streams. |
thisalihassan
left a comment
thisalihassan
left a comment
There was a problem hiding this comment.
overall changes lg but we can improve tests I believe. I will wait someone from the node team to request CI and review the PR
|
Hi Node.js maintainers |
3 participants
Fixes a regression where
setupHistoryManager()could run beforethis.inputwas assigned, causing a crash whenonHistoryFileLoadedis present (for example, Proxy or
jest.mock()inputs).This change ensures
this.inputis initialized before history setupand adds a regression test to cover the scenario.
Refs: #61526
Testing: