[v6.x backport] repl: refactor LineParser implementation

[lance]

Backporting this PR to address #15704. In addition to the basic backport, I have added a test test/parallel/test-repl-multi-line-templates.js to test for the issue noted in the issue above.

Original pull request: #6171

Original commit message:

---

Move the core logic from LineParser should fail handling into the
recoverable error check for the REPL default eval.

PR-URL: #6171
Reviewed-By: James M Snell [email protected]
Reviewed-By: Michaël Zasso [email protected]

---

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• commit message follows commit guidelines

Affected core subsystem(s)

repl

[lance]

/cc @Fishrock123@MylesBorins

This supercedes the PR submitted earlier.

[lance]

CI: https://ci.nodejs.org/job/node-test-pull-request/10406/

[lance]

Lots of code-and-learn PRs to deal with, I know. Just keeping this visible. Ping @nodejs/backporting

[MylesBorins]

@lance the original PR was Semver-Major, how has this changed to make it Semver-Patch?

[lance]

@MylesBorins to be honest, I'm not sure how it should be handled. My initial reasoning for doing the backport is that it fixes #15704, which is an issue in 6.x.

If I'm reading git history/blame correctly (and I may not be), editor mode support landed in master after the line parser refactor implementation. However, when editor mode was backported to v6.x the lineParser refactoring had not been backported, so the fix noted above didn't come along for the ride.

So, I honestly don't know what the answer is. But it seems like this would be the most straightforward way to implement the fix. I initially began a PR that just pulled some relevant bits from the lineParser refactoring. But had the impression that this was not the best approach to take, so I did a full backport.

I'm ok with either approach, and would be happy to close this and reopen the former if that makes more sense.

[MylesBorins]

/cc @nodejs/lts to chime in

If we can land this as semver patch we can get it in the next release cycle, so no rush

[gibfahn]

However, when editor mode was backported to v6.x the lineParser refactoring had not been backported, so the fix noted above didn't come along for the ride.

Assuming I'm understanding this correctly, this is a fix for #15704, in which case I'd be fine with landing it. If it's not urgent there's no reason not to wait till next release though.

@lance so to your knowledge this change itself isn't semver-major?

[lance]

@gibfahn it doesn't appear to be semver-major to me. To be honest, I'm not sure why the original PR was labeled that way, since it was simply an internal refactoring of LineParser and should not have affected the user experience in any way. I believe @Fishrock123 tagged it that way, so maybe he can shed some light on the thinking there.

But to be clear, yes, this is a fix for #15704.

[lance]

CI2: https://ci.nodejs.org/job/node-test-pull-request/10972/

Update new CI looks ok in spite of some unrelated raspberry pi test failures.

[lance]

Hmm - CI failed badly. Odd. Trying again: https://ci.nodejs.org/job/node-test-pull-request/11119/

Update: An unrelated Windows failure, and a couple of the regular raspberry pi failures.

[MylesBorins]

@nodejs/lts I've landed this in 6ca3640
please lmk if this should be reverted

[MylesBorins]

@jasnell@Fishrock123 you two originally labelled this change semver major. do you stand by that original review? should I back this out?

[MylesBorins]

Hey @lance I had to back this out of v6.x as it is breaking coffeescript 😢

[lance]

@MylesBorins ok - what's next? I'd like to find a fix for #15704 but I guess this won't be it.

[MylesBorins]

@lance if we can find a way to do this that doesn't break ecosystem modules we can land it

[lance]

@MylesBorins is there a known way to reproduce the errors that were seen with coffeescript?

[MylesBorins]

Simply running the test suite can reproduce the error

…

On Nov 27, 2017 11:44 PM, "Lance Ball" ***@***.***> wrote: @MylesBorins <https://github.com/mylesborins> is there a known way to reproduce the errors that were seen with coffeescript? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#15773 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAecV_L-ypZu4Bp5Na0yovVAlUAiM5q9ks5s6srBgaJpZM4PuBPI> .

[BridgeAR]

How shall we continue here?

[lance]

@BridgeAR I'm sorry I have been quite consumed with Red Hat related work lately and have not had an opportunity to get back to this. As mentioned in the initial comment, it is meant to address #15704. I do not have a good sense of how important this issue is and whether a backport is really necessary. If so, I assume we'd want it before active LTS ends in April?

[BridgeAR]

I personally would say it is not as important but I do not have a strong opinion in this case.

@MylesBorins what do you think?

[MylesBorins]

As the error only exists in the repl and I have not seen other people report the issue, I think it is reasonable to close this if @lance doesn't have time to dig in

[lance]

OK - I will close this for now. If I get a little breathing room to pick it up again I may.