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

Prevent "meteor shell" from stomping Underscore #3227

Merged
merged 1 commit into from Dec 4, 2014
Merged

Conversation

@graemian
Copy link
Contributor

@graemian graemian commented Dec 4, 2014

The return value of last REPL command is stored in global._ if the REPL is set to use the global context. So after you run your first REPL command, Underscore is no longer available to Meteor server code as global._ - that variable is now the result of your last REPL command.

See http://stackoverflow.com/questions/10973968/underscore-doesnt-work-in-coffeescripts-console

The return value of last command is stored in global._ if the REPL is set to use the global context. So Underscore is no longer available to Meteor server code as global._

See http://stackoverflow.com/questions/10973968/underscore-doesnt-work-in-coffeescripts-console
@benjamn
Copy link
Member

@benjamn benjamn commented Dec 4, 2014

Oh, fascinating!

benjamn added a commit that referenced this pull request Dec 4, 2014
Prevent "meteor shell" from stomping Underscore.
@benjamn benjamn merged commit 7c7e52f into meteor:devel Dec 4, 2014
1 check passed
1 check passed
@apollo-cla
default The author has signed the Meteor Contributor Agreement.
Details
@Slava
Copy link
Member

@Slava Slava commented Dec 4, 2014

Cool, we were discussing this just yesterday with Sashko :)

@benjamn
Copy link
Member

@benjamn benjamn commented Dec 4, 2014

Note that our implementation of evalCommand uses the global object as its context unconditionally (i.e. useGlobal: true is blindly assumed), but the repl module does pay attention to useGlobal when it decides whether to define the _ variable.

In other words, this change is nice because commands are still executed in global scope (so you can define top-level functions etc.). The only difference is that _ is not set by the REPL after each command, which is precisely what we wanted.

Now, you might argue that having some way of accessing the last expression would be useful, but that's another discussion (that I'm happy to have here or elsewhere).

@Slava
Copy link
Member

@Slava Slava commented Dec 4, 2014

I like this behavior much better than repl overriding my precious underscore.

@graemian
Copy link
Contributor Author

@graemian graemian commented Dec 5, 2014

I was surprised that the fix worked, thanks for clarifying why @benjamn

I think the REPL guys need to give us a way to specify which variable we want the result of the last command to be returned in. I ain't gonna rename my Underscore for no-one ;-)

benjamn added a commit that referenced this pull request Jan 15, 2015
Summary:
Before this commit you could type `Meteor.is` in a `meteor shell` session
and then tab to see a list of possible completions (e.g.
`Meteor.isClient`, `Meteor.isServer`), but typing a prefix of a global
variable name like `Mete` followed by tab has been broken ever since we
stopped using the global object as the REPL context:
7c7e52f

The reason for that commit was to prevent the REPL from overwriting the
global `_` variable (which most Meteor developers expect to be bound to
`require("underscore")`): https://github.com/meteor/meteor/3227

This commit solves #3227 by making `repl.context._` a read-only property
that is permanently "bound" to underscore.  As a bonus, we now intercept
assignments to `_` and store those values as `repl.context.__`, so you
still have access to the last result in the shell via `__`.

Test Plan:
Run `meteor shell`, evaluate a few expressions, and see that (1) global
variables can be tab-completed, (2) `_` remains bound to underscore, and
(3) `__` gets bound to the result of the evaluated expressions.

Reviewers: avital, stubailo, glasser

Reviewed By: glasser

Differential Revision: https://phabricator.meteor.io/D12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants