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

Rewrite getNewUnsafeGlobal #204

Merged
merged 2 commits into from Mar 15, 2019

Conversation

Projects
None yet
4 participants
@katelynsills
Copy link
Contributor

katelynsills commented Mar 14, 2019

Background

Based on feedback from @ljharb and @warner, this PR replaces #203.

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 after bundling, isNode and isBrowser are both true due to various rewriting and shimming done by the bundlers. For example, webpack rewrites the platform detection section to:

  const isNode =  true && typeof module !== 'undefined';
  const isBrowser = typeof document === 'object';

and defines module (document is already defined.)

Description

Key insight: we don't actually care about detecting whether the code is being run in Node.js or in the browser. We care about whether we are getting the appropriate newUnsafeGlobal. So, instead of relying on mechanisms for trying to detect the platform which may turn out to be fragile after being handled by a bundler, we skip the detection and go ahead and call the methods createNewUnsafeGlobalForBrowser and createNewUnsafeGlobalForNode. Each is wrapped in their own try/catch. We retain the same logic as before - if neither or both succeed, something is very wrong and we throw an error. If one succeeds, we go forward and use the return value as newUnsafeGlobal.

To satisfy the bundlers, we still have to conditionally require('vm'). The bundlers shim vm, so unless we have a check, the bundlers will try to proceed using their shimmed version of vm.

In the future, if we had the shim as a separate repository and generated files that were pointed to by pkg.browser, pkg.module, etc. we might be able to get rid of this conditional require of vm.

Closes #188

Assumptions

  • This is first-run code
  • We do not expect downstream users to add specific config settings to their bundlers (i.e. turning off shimming of built-in Node.js modules.)

Testing

npm run shim:test
The SES bundler tests are able to confirm that this PR solves this particular problem.

@erights
Copy link
Collaborator

erights left a comment

Kate and I just reviewed this f2f. LGTM

@ljharb

ljharb approved these changes Mar 14, 2019

@warner warner merged commit 639fcfb into tc39:master Mar 15, 2019

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.