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 error message #13733

Closed
wants to merge 4 commits into from
Closed

Conversation

BridgeAR
Copy link
Member

@BridgeAR BridgeAR commented Jun 17, 2017

While looking at the ERR_INVALID_ARG_TYPE errors, I stumbled across this one. The property name should be passed to the internal error instead of the type.

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

@nodejs-github-bot nodejs-github-bot added the repl Issues and PRs related to the REPL subsystem. label Jun 17, 2017
@tniessen tniessen added the errors Issues and PRs related to JavaScript errors originated in Node.js core. label Jun 17, 2017
@tniessen
Copy link
Member

tniessen commented Jun 17, 2017

Side note: There is an error code ERR_INVALID_REPL_HISTORY which appears to be unused but would apply here. Is this correct?

@BridgeAR
Copy link
Member Author

Good catch. It is indeed not used and it is very similar to the ERR_INVALID_ARG_TYPE error, so I went ahead and removed the ERR_INVALID_REPL_HISTORY error.

@tniessen
Copy link
Member

I am actually not too sure whether this is correct as it is. Considering that repl.history is not an argument or option of any kind but rather constructed from the contents of a file, ERR_INVALID_REPL_HISTORY might be the better option. I traced its introduction to #13299.

/cc @addaleax @Fishrock123

@addaleax
Copy link
Member

addaleax commented Jun 18, 2017

I think what @tniessen says makes sense (but tbh I’ve never actually seen this error happen in the wild)

@BridgeAR
Copy link
Member Author

I changed the error to ERR_INVALID_REPL_HISTORY as suggested.

@BridgeAR BridgeAR mentioned this pull request Jun 21, 2017
3 tasks
@tniessen tniessen self-assigned this Jun 21, 2017
@tniessen tniessen added the semver-major PRs that contain breaking changes and should be released in the next major version. label Jun 21, 2017
@tniessen
Copy link
Member

I think this should be semver-major due to the changed error code.

@jasnell Are you okay with this? Gonna land it then.

@BridgeAR
Copy link
Member Author

BridgeAR commented Jun 21, 2017

Wait, this should not be merged as is. The error is actually completely obsolete and it does not matter what is thrown as the catch block is going to catch the error and return a new and different one that has nothing to do with this error.
It's also not semver major at the moment because the error was never exposed to the user.

I'm thinking about fixing it like this:

try {
  // Pre-v3.0, repl history was stored as JSON.
  // Try and convert it to line separated history.
  const oldReplJSONHistory = fs.readFileSync(oldHistoryPath, 'utf8');

  // Only attempt to use the history if there was any.
  if (oldReplJSONHistory) repl.history = JSON.parse(oldReplJSONHistory);
} catch (err) {
  return ready(
    new errors.Error('ERR_PARSE_HISTORY_DATA', oldHistoryPath));
}

if (!Array.isArray(repl.history)) {
  return ready(new errors.TypeError('ERR_INVALID_REPL_HISTORY',
                                    typeof repl.history));
}
repl.history = repl.history.slice(0, repl.historySize);

This also needs tests as they are missing and lead to the error in the first place.

What do you all think?

Note to myself: check #2449

@tniessen
Copy link
Member

You are right, I did not even look at that before. Knowing this it makes even less sense why there is ERR_PARSE_HISTORY_DATA and ERR_PARSE_HISTORY_DATA. I am okay with your suggestion, even though I kind of dislike having two different error codes for more or less the same problem. Still, I think your solution might be the closest to what the original authors were trying to achieve.

@BridgeAR
Copy link
Member Author

I fixed the error handling in a way that I expect was originally meant. I am not a big fan of it though and while working on it I thought it might be worth thinking about simple removing the old history file, so I opened an alternative: #13876

@BridgeAR
Copy link
Member Author

As suggested in #13876 the way would be to make a end of life deprecation and then remove this part overall. But until then this fix could go into the code base and I think it could even be backported as well and is a semver-patch.

So PTAL

@tniessen
Copy link
Member

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

@jasnell @lpinca @cjihrig There have been some major changes since your approvals, could you PTAL and just give me a thumb up if you are okay with landing this as it is?

@cjihrig
Copy link
Contributor

cjihrig commented Jun 28, 2017

This has conflicts.

@BridgeAR
Copy link
Member Author

Rebased

@lpinca
Copy link
Member

lpinca commented Jun 29, 2017

@@ -124,6 +125,7 @@ function setupHistory(repl, historyPath, oldHistoryPath, ready) {
}

function onread(err, data) {
var threw;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: why not putting this in the if branch where the try...catch is defined?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

// Grab data from the older pre-v3.0 JSON NODE_REPL_HISTORY_FILE format.
_writeToOutput(
repl,
'\nConverting old JSON repl history to line-separated history.\n' +
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: I don't think this is a problem but this message is now displayed after the conversion is actually done.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was intentional as the message would otherwise be visible even though the conversion did not occur due to e.g. ENOENT or a parsing error etc.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I'm fine with this it's just that "converting" suggests that conversion is still happening so it makes sense to display it even if there is a parse error.

I would use "Converted" now.

Copy link
Member Author

@BridgeAR BridgeAR Jun 29, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed

@BridgeAR
Copy link
Member Author

By the way: IMHO this should not be semver-major (if by any means, then because of the change from converting to converted and in that case I'd rather revert that change). The former handling did not work as expected and this is a patch for that.

@tniessen
Copy link
Member

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

Please fix CI failures.

@BridgeAR
Copy link
Member Author

Fixed

@tniessen
Copy link
Member

tniessen commented Jun 30, 2017

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

I will land this after giving @nodejs/ctc some time to review and decide semver-ity (even though - technically - two CTC members have already approved).

@addaleax
Copy link
Member

addaleax commented Jun 30, 2017

As far as I can tell this only affects the REPL, as in, the application built into Node, not the public repl module, so semver-patch seems fine.

@tniessen tniessen removed the semver-major PRs that contain breaking changes and should be released in the next major version. label Jun 30, 2017
Trott pushed a commit to Trott/io.js that referenced this pull request Jun 30, 2017
PR-URL: nodejs#13733
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
@tniessen
Copy link
Member

tniessen commented Jun 30, 2017

Landed in e6b69b9. Thank you for your contribution!

CIs on master: LinuxOne, Windows

@tniessen tniessen closed this Jun 30, 2017
@addaleax
Copy link
Member

addaleax commented Jul 3, 2017

This doesn’t land cleanly on 8.x; if you can, please follow the guide and raise a backport PR, if you don’t think it’s worth it (or wrong) let me know and we’ll add the dont-land-on label.

@addaleax
Copy link
Member

@lpinca @jasnell @cjihrig @tniessen Do any of you have the time to backport?

BridgeAR added a commit to BridgeAR/node that referenced this pull request Jul 20, 2017
PR-URL: nodejs#13733
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
@BridgeAR
Copy link
Member Author

@addaleax I opened a backport to 8.x

addaleax pushed a commit that referenced this pull request Jul 21, 2017
Backport-PR-URL: #14392
Backport-Reviewed-By: James M Snell <jasnell@gmail.com>

PR-URL: #13733
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Fishrock123 pushed a commit that referenced this pull request Jul 24, 2017
Backport-PR-URL: #14392
Backport-Reviewed-By: James M Snell <jasnell@gmail.com>

PR-URL: #13733
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
@addaleax addaleax mentioned this pull request Jul 24, 2017
@addaleax addaleax mentioned this pull request Aug 2, 2017
@MylesBorins
Copy link
Contributor

Should this be backported to v6.x-staging? If yes please follow the guide and raise a backport PR, if no let me know or add the dont-land-on label.

@BridgeAR
Copy link
Member Author

I do not think that it is worth backporting. So I changed the label accordingly.

@BridgeAR BridgeAR deleted the fix-error-msg branch April 1, 2019 23:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
errors Issues and PRs related to JavaScript errors originated in Node.js core. repl Issues and PRs related to the REPL subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants