-
-
Notifications
You must be signed in to change notification settings - Fork 535
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
Fix REPL bug with previous line carryover #1480
Fix REPL bug with previous line carryover #1480
Conversation
Codecov Report
|
src/repl.ts
Outdated
// and adding a semicolon to the end of a successful input won't ever change the output. | ||
// We check to make sure the previous line ends in a newline just in case, even though it should | ||
// always be the case. | ||
if (/\n$/.test(state.input)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A detailed explanation within the code! Love it; thanks for this.
Thanks for sending this. I haven't looked at this code in a bit, but I noticed coverage of one line being removed by this change. https://app.codecov.io/gh/TypeStrong/ts-node/compare/1480/changes#D1L605 Is it possible that the line in question is trying to do the same thing as this PR, but not doing it well enough? Again, I haven't looked at this code in a bit, so I don't know off-hand. |
Also, we should add a test for this, since none of our tests currently are catching the bug being fixed here. I recently made an effort to make our tests more approachable by splitting them into several files. Not perfect, still a work-in-progress. But REPL tests are here: |
Looks like that lost line of coverage was from the code attempting to fix this same sort of issue, but it seems like the motivation was probably slightly different at the time and so it had an overly restrictive condition. Since the new fix appears to subsume whatever that line was intended to do, I've removed that code. |
Ok sounds good, thanks. My only remaining concern: The old code was inside For example, I think the If what I'm saying makes sense, can we move your code to where the old code was? |
Moving the code to
Looking more closely at the code, it seems like it was trying to do this fix for things like Adding The other two situations that use
There was an issue where |
…-semicolons as empty statements and this *might* affect stack trace line numbers at some point
… console.log() capture
Thanks for the detailed analysis. Sounds good to me. The only thing I changed: |
@TheUnlocked this has been published in 10.3.0. Thanks again! https://github.com/TypeStrong/ts-node/releases/tag/v10.3.0 |
Fixes #1363.
Multiline inputs aren't an issue since multiline inputs are passed into
evalCodeInternal
in their entirety.