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

Shell: Don't be confused by SyntaxErrors thrown by command execution. #8446

Merged
merged 3 commits into from Mar 8, 2017

Conversation

@mattmccutchen
Copy link
Contributor

@mattmccutchen mattmccutchen commented Mar 2, 2017

Fixes #8290.

Do we need a test for this? I think it's pretty unlikely that someone will regress this unintentionally.

Apply the check for a SyntaxError indicating an incomplete command only
to the ECMAScript translation and parsing of a command and not to the
execution, following the example of Node's defaultEval function.

Fixes #8290.
@mattmccutchen mattmccutchen force-pushed the mattmccutchen:shell-syntaxerror branch to 28309de Mar 2, 2017
Just a note to revisit this/clean-up in Node 6 as the `Recoverable` is now exported from Node 6's `eval` and this is no longer necessary.

* https://github.com/nodejs/node/blob/v6.x/lib/repl.js#L1398
* https://nodejs.org/api/repl.html#repl_custom_evaluation_functions
@abernix
Copy link
Member

@abernix abernix commented Mar 3, 2017

This is awesome! Thanks for looking into this, @mattmccutchen!

Just to provide insight to anyone peering in: Some of this repl custom eval functionality has been properly exposed and documented in Node 6.x, so we'll need to change this again at that time, but this seems mostly on par with those new docs already (except for us capturing the RecoverableError constructor ourselves instead of using the eval.Recoverable which is now available).

Re: Tests. I think all regressions are unintentional and I'd like to get tests for this eventually, but seeing at there is not currently a harness in place for the shell (an interactive test is necessary, perhaps) and this is actually an improvement to an existing deficiency, we might be willing to forgo a test for this. That being said, I'll leave this for more "LGTM"s as a precaution.

@hwillson
Copy link
Member

@hwillson hwillson commented Mar 3, 2017

I agree with @abernix - since there isn't currently a defined automatic testing process for the shell, LGTM! Thanks @mattmccutchen!

@abernix abernix added this to the Release 1.4.3.x milestone Mar 8, 2017
@abernix
abernix approved these changes Mar 8, 2017
@benjamn benjamn merged commit 525568c into meteor:devel Mar 8, 2017
2 checks passed
2 checks passed
CLA Author has signed the Meteor CLA.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@benjamn
Copy link
Member

@benjamn benjamn commented Mar 8, 2017

Thanks @mattmccutchen (and @abernix)!

@abernix abernix modified the milestones: Release 1.4.3.x, Release 1.4.3.2 Mar 9, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants