repl: Fix debugger / require bugs - #1605
Conversation
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
|
Tested all three mentioned issues and they seem to be fixed. Not too familiar with the repl code, but functionality wise, this is LGTM. |
Sorry, something went wrong.
|
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 :) |
Sorry, something went wrong.
|
Thanks for the review. Merging this now. |
Sorry, something went wrong.
|
Please do, I just wanted to merge myself 👍 |
Sorry, something went wrong.
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 <me@silverwind.io>
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 <me@silverwind.io>
Sorry, something went wrong.
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 addressed require('./local-file.js') from the repl; it actually addressed require('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.)