Mocha Breaks Libraries Using process for environment detection. #770

Closed
iammerrick opened this Issue Mar 12, 2013 · 19 comments

Comments

Projects
None yet
10 participants
@iammerrick
Contributor

iammerrick commented Mar 12, 2013

Please see an example repository at https://github.com/iammerrick/mocha-rjs-fail.

The Problem

Mocha.js installs a global called process see here. A comment says it is only to allow mocha.js to run untouched, not to allow running node code in the browser, however this has some unfortunate side effects for some libraries that use process for environment detection.

r.js is one of those libraries that uses process for environment detection see here.

@tj

This comment has been minimized.

Show comment
Hide comment
@tj

tj Mar 12, 2013

Contributor

hmm yeah that sort of thing gets messy, we should be able to pretty easily avoid that if we do our own checking

Contributor

tj commented Mar 12, 2013

hmm yeah that sort of thing gets messy, we should be able to pretty easily avoid that if we do our own checking

@iammerrick

This comment has been minimized.

Show comment
Hide comment
@iammerrick

iammerrick Mar 12, 2013

Contributor

Does that mean the browser doesn't use these lines of code: https://github.com/visionmedia/mocha/blob/master/mocha.js#L5229-L5290

Also thanks for your quick response.

M

Contributor

iammerrick commented Mar 12, 2013

Does that mean the browser doesn't use these lines of code: https://github.com/visionmedia/mocha/blob/master/mocha.js#L5229-L5290

Also thanks for your quick response.

M

@tj

This comment has been minimized.

Show comment
Hide comment
@tj

tj Mar 12, 2013

Contributor

actually maybe what we can do is just wrap those in the anonymous closure there so that we're not leaking process, the rest would still be the same, that should be fine

Contributor

tj commented Mar 12, 2013

actually maybe what we can do is just wrap those in the anonymous closure there so that we're not leaking process, the rest would still be the same, that should be fine

@iammerrick

This comment has been minimized.

Show comment
Hide comment
@iammerrick

iammerrick Mar 12, 2013

Contributor

I think its in a closure, what we really need is a var statement on https://github.com/visionmedia/mocha/blob/master/mocha.js#L5237. Right?

Contributor

iammerrick commented Mar 12, 2013

I think its in a closure, what we really need is a var statement on https://github.com/visionmedia/mocha/blob/master/mocha.js#L5237. Right?

@curvedmark

This comment has been minimized.

Show comment
Hide comment
@curvedmark

curvedmark May 8, 2013

Contributor

Just want to let you guys know that this change breaks mocha-phantomjs nathanboktae/mocha-phantomjs#58, which need to override some methods in process.

I guess one way to fix this is to expose process in the browser with Mocha.process, and mocha-phantomjs should use that instead.

Not sure what @visionmedia and @metaskills think about this.

Contributor

curvedmark commented May 8, 2013

Just want to let you guys know that this change breaks mocha-phantomjs nathanboktae/mocha-phantomjs#58, which need to override some methods in process.

I guess one way to fix this is to expose process in the browser with Mocha.process, and mocha-phantomjs should use that instead.

Not sure what @visionmedia and @metaskills think about this.

@metaskills

This comment has been minimized.

Show comment
Hide comment
@metaskills

metaskills May 8, 2013

Agreed, something should happen. Even when PhantomJS 1.9.1 comes out with proper std{out,error} support, see my issue 50, we will see need to capture Mocha's process.stdout.write so we can push it back up the proper PhantomJS IO stream.

Agreed, something should happen. Even when PhantomJS 1.9.1 comes out with proper std{out,error} support, see my issue 50, we will see need to capture Mocha's process.stdout.write so we can push it back up the proper PhantomJS IO stream.

@tj

This comment has been minimized.

Show comment
Hide comment
@tj

tj May 22, 2013

Contributor

hmmm tricky.. if we get too cute there it's going to be one huge PITA to maintain, Mocha.process is pretty ugly but I guess it would do

Contributor

tj commented May 22, 2013

hmmm tricky.. if we get too cute there it's going to be one huge PITA to maintain, Mocha.process is pretty ugly but I guess it would do

@mourner

This comment has been minimized.

Show comment
Hide comment
@mourner

mourner Jun 5, 2013

+1, had to revert to Mocha 1.9.0 to use it with PhantomJS and grunt-mocha-phantomjs grunt plugin...

mourner commented Jun 5, 2013

+1, had to revert to Mocha 1.9.0 to use it with PhantomJS and grunt-mocha-phantomjs grunt plugin...

@tj

This comment has been minimized.

Show comment
Hide comment
@tj

tj Jun 5, 2013

Contributor

I'll close this one since it has been merged, but we'll need to tweak for the phantom stuff

Contributor

tj commented Jun 5, 2013

I'll close this one since it has been merged, but we'll need to tweak for the phantom stuff

@tj tj closed this Jun 5, 2013

@iammerrick

This comment has been minimized.

Show comment
Hide comment
@iammerrick

iammerrick Jun 5, 2013

Contributor

The grunt-mocha-phantom was built on top of a hack. My vote is we expose using Mocha.process.

Contributor

iammerrick commented Jun 5, 2013

The grunt-mocha-phantom was built on top of a hack. My vote is we expose using Mocha.process.

@tj

This comment has been minimized.

Show comment
Hide comment
@tj

tj Jun 5, 2013

Contributor

even having Mocha.process is pretty hacky too :( necessary I suppose, maybe we can formalize a nicer api for intercepting that data

Contributor

tj commented Jun 5, 2013

even having Mocha.process is pretty hacky too :( necessary I suppose, maybe we can formalize a nicer api for intercepting that data

@metaskills

This comment has been minimized.

Show comment
Hide comment
@metaskills

metaskills Jun 5, 2013

Would love anything formalized. Either way, thanks everyone!

Would love anything formalized. Either way, thanks everyone!

@metaskills

This comment has been minimized.

Show comment
Hide comment
@metaskills

metaskills Jun 14, 2013

close this one... but we'll need to tweak for the phantom stuff

Is there a new issue that is tracking that?

close this one... but we'll need to tweak for the phantom stuff

Is there a new issue that is tracking that?

@dominicbarnes

This comment has been minimized.

Show comment
Hide comment
@dominicbarnes

dominicbarnes Jun 19, 2013

Contributor

+1 for Mocha.process as well, I would really like to be able to upgrade to newer versions of mocha with mocha-phantomjs. :)

Contributor

dominicbarnes commented Jun 19, 2013

+1 for Mocha.process as well, I would really like to be able to upgrade to newer versions of mocha with mocha-phantomjs. :)

@johnaschroeder

This comment has been minimized.

Show comment
Hide comment
@johnaschroeder

johnaschroeder Jun 20, 2013

+1, would also like to be able to upgrade to newer versions of mocha with mocha-phantomjs.

+1, would also like to be able to upgrade to newer versions of mocha with mocha-phantomjs.

@dominicbarnes dominicbarnes referenced this issue in nathanboktae/mocha-phantomjs Jun 20, 2013

Closed

Cannot Run on CLI (process variable undefined errors) #58

@alexgorbatchev

This comment has been minimized.

Show comment
Hide comment

👍

@mallim

This comment has been minimized.

Show comment
Hide comment

mallim commented Jun 25, 2013

👍

@metaskills

This comment has been minimized.

Show comment
Hide comment
@metaskills

metaskills Jun 25, 2013

Please, I do not want this to be an up vote thread.

@visionmedia Is there another issue that has opened up for this or is this something you are not working on? If so, can you give anyone feedback on what would be OK by you for someone to work on and create a new pull request.

Please, I do not want this to be an up vote thread.

@visionmedia Is there another issue that has opened up for this or is this something you are not working on? If so, can you give anyone feedback on what would be OK by you for someone to work on and create a new pull request.

@diwu1989

This comment has been minimized.

Show comment
Hide comment
@diwu1989

diwu1989 Jul 1, 2013

I put in this pull request just now #916

It exposes the shim process that Mocha uses as Mocha.process.

diwu1989 commented Jul 1, 2013

I put in this pull request just now #916

It exposes the shim process that Mocha uses as Mocha.process.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment