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

Correct global _ destruction via `meteor shell`. #9406

Merged
merged 10 commits into from Dec 2, 2017

Conversation

Projects
None yet
3 participants
@abernix
Member

abernix commented Nov 21, 2017

(PR body is the commit message body from 15b24ff.)

A truthy useGlobal option when calling Node's repl.start() will use the global context, which will allow global Meteor variables (such as the global _ provided by the underscore) to be available in the context of the REPL shell. This sounds desirable, and in fact, the original
implementation set it as such, only to be later changed to false in 7c7e52f, specifically to avoid the undesirable behavior of Node REPL stomping on the global _ variable with its own special-meaning _ variable which is set to the value of the most recently eval'd expression.

This useGlobal was once again changed to true to fix the lost tab-completion functionality, thanks to 2443d83, which also implelented special mutator on _ to avoid breaking it. As it's probably clear now, this has been a struggle, and because of Node using its own Object.defineProperty('_', ...) as of Node.js 6 (in node/node#5535), this problem has surfaced again, as reported in #9276.

This changes the behavior to another implementation which starts with useGlobal set to false, but then sets the context immediately to global, which seems to avoid the problem.

This does maintain the existing behavior of our special double-underscore variable, in order to get the value of the last REPL expression, in the same manner as _ would in a normal REPL, by
accessing the internal last property. Since this is also undocumented (on our end), this seemed reasonable.


I'd recommend reviewing this PR at one time, in it's entirety, but keep in mind that the individual commits provide additional commentary in their own commit messages (and changes).

I attempted to make tests for this, but since the self tests seem to invoke the evaluateAndExit functionality within meteor shell (due to their lack of a full TTY), the context is not affected in any way.

Fixes #9276.

abernix added some commits Nov 21, 2017

Use the (now) `repl`-exported `Recoverable` error.
In order to catch so-called "recoverable" REPL errors (for example,
commands in the process of being inputted which are syntax errors until
they are completed/recovered), it had previously been necessary to
intentionally cause such an error and save the error's prototype for
future use.

Node 6 changed this by exporting the Error directly from the `repl`
module, and this commit represents the changes necessary to remove that
(now unnecessary) behavior.

This also takes the opportunity to update the `isRecoverableError` (and
related) functions which had previously been borrowed from the `repl`
module source, to their newer versions.
Stop stripping parens from ES classes used in the REPL.
As Node.js 8 natively supports ECMAScript classes, this code shouldn't
be necessary anymore.
Again, correct the context of `_` within `meteor shell`.
A truthy `useGlobal` option when calling Node's `repl.start()` will use
the `global` context, which will allow global Meteor variables (such as
the global `_` provided by the `underscore`) to be available in the context
of the REPL shell.  This sounds desirable, and in fact, the original
implementation set it as such, only to be later changed to `false` in
7c7e52f, specifically to
avoid the undesirable behavior of Node REPL stomping on the global `_`
variable with its own special-meaning `_` variable which is set to the
value of the most recently eval'd expression.

This was once again changed to `true` to fix the lost tab-completion
functionality, thanks to 2443d83,
which also implelented special mutator on `_` to avoid breaking
it.  As it's probably clear now, this has been a struggle, and because
of Node using its own `Object.defineProperty('_', ...)` as of Node.js 6
(in https://github.com/nodejs/node/pull/5535/files), this problem has
surfaced again, as reported in #9276.

This changes the behavior to another implementation which starts with
`useGlobal` set to `false`, but then sets the context immediately to
`global`, which seems to avoid the problem.

It's possible that another option might be to set explicitly set the
`underscoreAssigned` attribute on the `repl` after starting it, however
since that's an internal, undocumented property, it might be best to avoid.

This does maintain the existing behavior of our special
double-underscore variable, in order to get the value of the last
REPL expression, in the same manner as `_` would in a normal REPL, by
accessing the internal `last` property.  Since this is also undocumented
(on our end), this seemed reasonable.

@abernix abernix requested review from benjamn and hwillson Nov 21, 2017

@hwillson

hwillson approved these changes Nov 22, 2017 edited

Looks good to me @abernix! I've played around with these changes a bit, and I can confirm this fixes #9276. Actually, these changes also fix #5415. 2 for the price of 1 - Black Friday on Wednesday! 🙂

@hwillson

This comment has been minimized.

Member

hwillson commented Nov 22, 2017

Interesting - I didn't realize adding an issue link in a review comment doesn't register that link in the linked to issue thread. So, recapping here to fix the issue thread - these changes also fix #5415.

// https://github.com/meteor/meteor/commit/2443d832265c7d1c
Object.defineProperty(repl.context, "__", {
get: () => repl.last,
set: (val) => {

This comment has been minimized.

@benjamn

benjamn Nov 22, 2017

Member

When is repl.context.__ ever set?

This comment has been minimized.

@abernix

abernix Nov 22, 2017

Member

We don't, but I copied the behavior that Node.js uses in the native repl code located here:

https://github.com/nodejs/node/blob/ad8257fa5b9795d3d79fa4a91d0f18c43f024ab3/lib/repl.js#L575-L587

...in which they also allow setting of _, which gets stored on the repl instance's this.last property (in this code repl.last)

}
return stringLiteral ? lastChar === '\\' : isBlockComment;
}

This comment has been minimized.

@benjamn

benjamn Nov 22, 2017

Member

If there was any way to avoid copying this code here, I think that would make our REPL implementation much more maintainable in the future!

abernix added some commits Nov 27, 2017

Wrap default `repl` "eval" function, rather than duplicating logic.
Addresses feedback from @benjamn.

Rather than copying the `IsRecoverableError` and `isCodeRecoverable`
methods from the Node.js `repl` module source (in order to capture
so-called "Recoverable" errors), wrap the default "eval" function with
our relatively thin logic, thus avoiding the need to continually update
the definition of what's "recoverable" as Node's implementation evolves.
// of catching so-called "Recoverable Errors" (https://git.io/vbvbl),
// we will wrap the default eval, run it in a Fiber (via a Promise), and
// give it the opportunity to decide if the user is mid-code-block.
const defaultEval = repl.eval;

This comment has been minimized.

@benjamn

benjamn Nov 28, 2017

Member

Awesome!

abernix added some commits Nov 28, 2017

Remove unnecessary setting of `repl.context`.
This is superfluous residue that I inadvertently created when splitting the
existing `startREPL` function into `setupREPL` and `enableInteractiveMode`.

The context is already set in `setupREPL` (to the exact same value as
here) by the time that this occurrence in `enableInteractiveMode` is called.
Ensure that `require` and `module` are always set.
I previously had thought that a duplicate call to `setRequireAndModule`
encountered in code-path would no longer be necessary after some
consolidation in previous steps of this re-factor, but the test failure
seen here made it clear what was happening:

https://circleci.com/gh/meteor/meteor/12445

Specifically, if a module was imported in a piped command (that is to say,
when no TTY is present and the `evaluateAndExit` code-path is taken), as so:

    echo 'import { Meteor } from "meteor/meteor"' | meteor shell

...the `module` and `require` symbols were not set.  Conveniently, this is
the environment in effect when the `meteor self-test` suite is ran since
they do not have a TTY.

This moves the `setAndRequire` from the "interactive-only" function into
the general REPL setup and further harmonizes the code.

@benjamn benjamn added this to the Package Patches milestone Nov 29, 2017

abernix added some commits Nov 30, 2017

Test which ensures that the global `_` isn't compromised.
As demonstrated in #9276.

This test wouldn't have caught the regression in the previous solution
since the lack of a TTY in the `self-test` test harness caused the tests
themselves to take the path through `shell-server`'s `evaluateAndExit`
logic, which didn't use the `global` scope in the same way as the
interactive shell.  That is no longer the case as of e0682c5.
Only import `start` from `repl` instead of the entire module.
It was previously necessary to have more from the `repl` module, but
it's sufficient to just have `start` now since we wrap the default
`eval`.

@benjamn benjamn merged commit 855dfe8 into devel Dec 2, 2017

13 checks passed

CLA Author has signed the Meteor CLA.
Details
ci/circleci: Get Ready Your tests passed on CircleCI!
Details
ci/circleci: Group 0 Your tests passed on CircleCI!
Details
ci/circleci: Group 1 Your tests passed on CircleCI!
Details
ci/circleci: Group 2 Your tests passed on CircleCI!
Details
ci/circleci: Group 3 Your tests passed on CircleCI!
Details
ci/circleci: Group 4 Your tests passed on CircleCI!
Details
ci/circleci: Group 5 Your tests passed on CircleCI!
Details
ci/circleci: Group 6 Your tests passed on CircleCI!
Details
ci/circleci: Group 7 Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment