Skip to content
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

repl: Fix debugger / require bugs #1605

Closed
wants to merge 3 commits into from
Closed

repl: Fix debugger / require bugs #1605

wants to merge 3 commits into from

Conversation

chrisdickinson
Copy link
Contributor

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.)

@chrisdickinson chrisdickinson added debugger repl Issues and PRs related to the REPL subsystem. labels May 4, 2015
@chrisdickinson chrisdickinson added this to the 2.0.0 milestone May 4, 2015
@chrisdickinson
Copy link
Contributor Author

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.

@chrisdickinson
Copy link
Contributor Author

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.

@silverwind
Copy link
Contributor

Tested all three mentioned issues and they seem to be fixed. Not too familiar with the repl code, but functionality wise, this is LGTM.

@silverwind
Copy link
Contributor

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 :)

@chrisdickinson
Copy link
Contributor Author

Thanks for the review. Merging this now.

@silverwind
Copy link
Contributor

Please do, I just wanted to merge myself 👍

chrisdickinson added a commit that referenced this pull request May 4, 2015
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]>
chrisdickinson added a commit that referenced this pull request May 4, 2015
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]>
@chrisdickinson
Copy link
Contributor Author

Merged in ca219b0 and 051d482.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
repl Issues and PRs related to the REPL subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants