repl: Fix debugger / require bugs #1605
Conversation
|
Added another commit to this – it looks like I was truncating the history file on load, and if a user didn't enter any lines before closing they'd lose history. |
|
I'm going to be AFK for about an hour starting now. If this LGTY, please merge it. Otherwise if you want to adopt the PR also feel free. I'll plan on being available again by 10:40PST. |
|
Tested all three mentioned issues and they seem to be fixed. Not too familiar with the repl code, but functionality wise, this is LGTM. |
|
One minor issue, which shouldn't block the release is that the cursor seems to momentarily reset to position 0 on the line while switching through history entries, at least in iterm2. This may have been there before persistency though :) |
|
Thanks for the review. Merging this now. |
|
Please do, I just wanted to merge myself 👍 |
The _debugger module uses the internal REPL module, but expects to receive the userland REPL module. This fixes the breakage that occurs by proxying the userland REPL module through the internal module. It also fixes an unintended in-REPL bug, where require(node-module) was not resolving correctly. PR-URL: #1605 Reviewed-By: Roman Reiss <[email protected]>
The second step of augmenting the internal REPL with persistent history was to re-open the history file with a 'w' handle. This truncated the file. If a user did not enter a new line before closing the REPL, their history would be deleted. PR-URL: #1605 Reviewed-By: Roman Reiss <[email protected]>
Hey all – found some (embarrassing) bugs I introduced while poking at the v2.0.0 nightly.
The first commit fixes the debugger – it started getting the new internal repl module, which did not expose the same interface as the public repl module. This is a quick fix, intended to be removed in the future – I'd like to spend some time on the debugger to bring it to parity with the new internal repl features.
The second commit fixes node_modules resolution support. This slipped past me the first time around, because I misread the purpose of the preamble (
hack for require.resolve("./relative") to work properly., from the source) and moved it to the internal module. I thought that it addressedrequire('./local-file.js')from the repl; it actually addressedrequire('dependency-from-node-modules'), and needed to be available to the repl eval function. This fixes that breakage. (And I also am heaping a ton of apologies on this one!)These should block v2.0.0 (or the repl improvements should be backed out.)