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

Fix definition of `isNode` and `isBrowser` to work with bundlers #203

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
2 participants
@katelynsills
Copy link
Contributor

katelynsills commented Mar 12, 2019

Description

The Realms shim currently fails in webpack, parcel, and browserify. (SES, which relies on Realms, has failing tests for these bundlers due to this problem here.) This prevents the Realms shim from being useful in many JavaScript workflows.

The error message is "unexpected platform, unable to create Realm," and this is because when the output of these bundlers is run in a browser, exports, module, and document are all objects, and thus isNode and isBrowser are both true.

This PR changes the definition of isNode and isBrowser. Instead of checking whether exports, module, and document are objects, this definition checks whether this is window or global. The check is wrapped in a try/catch such that a ReferenceError isn't thrown if window or global are undefined.

Closes #188

Tests

npm run shim:test

The SES bundler tests are able to confirm that this PR solves this particular problem.

@ljharb
Copy link
Member

ljharb left a comment

Both of my suggestions are bulletproof if this is first-run code; if not first-run code, your isBrowser is vulnerable to global.window = global;, and your isNode is vulnerable to window.global === window; (or globalThis.window = globalThis; globalThis.global = globalThis in either environment).

@@ -10,8 +10,8 @@ import { freeze } from './commons';
// this feature will be provided by the underlying engine instead.

// Platform detection.
const isNode = typeof exports === 'object' && typeof module !== 'undefined';
const isBrowser = typeof document === 'object';
const isBrowser = new Function('try {return this===window}catch(e){ return false}')(); // eslint-disable-line no-new-func

This comment has been minimized.

@ljharb

ljharb Mar 13, 2019

Member

why not typeof window !== 'undefined'?

const isNode = typeof exports === 'object' && typeof module !== 'undefined';
const isBrowser = typeof document === 'object';
const isBrowser = new Function('try {return this===window}catch(e){ return false}')(); // eslint-disable-line no-new-func
const isNode = new Function('try {return this===global}catch(e){ return false}')(); // eslint-disable-line no-new-func

This comment has been minimized.

@ljharb

ljharb Mar 13, 2019

Member

and why not typeof process !== 'undefined'?

This comment has been minimized.

@katelynsills

katelynsills Mar 14, 2019

Author Contributor

Webpack tries to shim process if it sees this check, so this fix gives the original error of "unexpected platform, unable to create Realm"

This comment has been minimized.

@ljharb

ljharb Mar 14, 2019

Member

Gotcha, that does make sense.

@ljharb

This comment has been minimized.

Copy link
Member

ljharb commented Mar 13, 2019

(a bundler might change global to window; it might shim process, but window won't be available in node in first-run code, and you can use the browser field to ensure that one entry point only runs in node, and another only in the browser, so i'm not sure why any "is browser" or "is node" logic is needed)

@katelynsills

This comment has been minimized.

Copy link
Contributor Author

katelynsills commented Mar 14, 2019

Thanks so much for the review, @ljharb. Based on your feedback and talking to @warner, I'm closing this and submitting #204 instead.

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.