/cc @Fishrock123 @MylesBorins

This supercedes the PR submitted earlier.

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

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

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

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

/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

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?

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

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.

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.

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

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

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

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

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

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

How shall we continue here?

@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?

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?

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

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