readline: initialize input before history manager #61538
Conversation
nodejs-github-bot
added
needs-ci
| 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; |
| 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. |
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: