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

[BUGFIX beta] Properly detect if the environment is Node. #11895

Merged
merged 1 commit into from
Jul 26, 2015

Conversation

brzpegasus
Copy link
Contributor

This fixes how we currently determine if the running environment is Node, by also ensuring that window is not defined. Simply checking for process is insufficient in browser environments such as those provided by NW.js or Electron, which also define a window.process.

I'll make a separate PR to rsvp to get it fixed there as well.

Closes #11679

Update: Relates to tildeio/rsvp.js#398

* Check for both `process` and `window` when determining if the environment is Node.
* Closes emberjs#11679.
@rwjblue
Copy link
Member

rwjblue commented Jul 25, 2015

Looks good to me.

@stefanpenner - Can you also review?

@stefanpenner
Copy link
Member

what about webworkers in those environments, do they also have global process?

@rwjblue
Copy link
Member

rwjblue commented Jul 25, 2015

@edeblois - Can you double check?

@brzpegasus
Copy link
Contributor Author

@stefanpenner Node is not available to web workers, both in NW.js and Electron. In those cases, you neither have window nor process (or global, require, etc.).

stefanpenner added a commit that referenced this pull request Jul 26, 2015
[BUGFIX beta] Properly detect if the environment is Node.
@stefanpenner stefanpenner merged commit 6e00835 into emberjs:master Jul 26, 2015
@rwjblue
Copy link
Member

rwjblue commented Jul 26, 2015

Thanks @brzpegasus!

@stefanpenner
Copy link
Member

@stefanpenner Node is not available to web workers,

awesome, this was the piece if information I was unclear about. Thanks for sharing.

@bastimeyer
Copy link

This is still broken in v1.13.6. It seems that this fix only went into the v2.0.0-beta.4 release...

@rwjblue
Copy link
Member

rwjblue commented Jul 31, 2015

@bastimeyer - Yep, that is what it was targeted for.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Loader issue since v1.13.0
4 participants