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

Ensure underscore can be run in a Node VM #2221

Merged
merged 1 commit into from Jul 6, 2015

Conversation

Projects
None yet
3 participants
@megawac
Copy link
Collaborator

megawac commented Jun 24, 2015

Discovered during caolan/async#804

@akre54

This comment has been minimized.

Copy link
Collaborator

akre54 commented Jun 25, 2015

I'm not sure I understand. Wouldn't you be using CommonJS (and thus module.exports instead of root) in a node environment? Why would you use _.noConflict() at all?

@@ -14,6 +14,34 @@

});

test('noConflict', function() {

This comment has been minimized.

@akre54

akre54 Jun 25, 2015

Collaborator

👍 this test

@megawac

This comment has been minimized.

Copy link
Collaborator Author

megawac commented Jun 25, 2015

Wouldn't you be using CommonJS (and thus module.exports instead of root) in a node environment?

No, this is for the node vm module (sandboxes)

Why would you use _.noConflict() at all?

You probably would't; the bigger issue is we're not correctly detecting the global, and thus the export for the vm, root._ = _; doesn't actually work.

@@ -14,6 +14,34 @@

});

test('noConflict', function() {
var underscore = _.noConflict();
equal(this._, void 0);

This comment has been minimized.

@michaelficarra

michaelficarra Jun 25, 2015

Collaborator

To avoid strict mode problems, test typeof _ instead.

@megawac megawac force-pushed the megawac:global-vm branch from 61d0c36 to f7af994 Jul 6, 2015

@megawac megawac force-pushed the megawac:global-vm branch from f7af994 to 7fc15e5 Jul 6, 2015

megawac added a commit that referenced this pull request Jul 6, 2015

Merge pull request #2221 from megawac/global-vm
Ensure underscore can be run in a Node VM

@megawac megawac merged commit dfe2307 into jashkenas:master Jul 6, 2015

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.26%) to 96.55%
Details

@megawac megawac deleted the megawac:global-vm branch Jul 6, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.