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

Electron's module wrapper differs from modern Node's (includes "process" and "global") #8358

Closed
ide opened this issue Jan 6, 2017 · 4 comments

Comments

@ide
Copy link
Contributor

ide commented Jan 6, 2017

  • Electron version: 1.4.11
  • Operating system: macOS 10.12.2

Expected behavior

I expected Electron to wrap JS modules in the same function wrapper as Node's. Electron defines an extra parameter ("process") which breaks code like const process = .... See Node's wrapper:

'(function (exports, require, module, __filename, __dirname) { '

https://github.com/nodejs/node/blob/0fb21df6e692bef9f55b9bfa876f3c59dc590332/lib/internal/bootstrap_node.js#L506

I tested with Node 6.0.0 & Node 7.4.0 and both use this wrapper.

Actual behavior

Electron uses a slightly different wrapper from its vendored copy of Node:

'(function (exports, require, module, __filename, __dirname, process, global) { '

https://github.com/electron/node/blob/811cfe3fcd360179d3dd436e3d80e1b045adf633/lib/internal/bootstrap_node.js#L489

How to reproduce

In a file loaded by Electron I have:

const child_process = require('child_process');
child_process.fork('./sample.js', [], {
    env: {
      ...process.env,
      ELECTRON_RUN_AS_NODE: 1,
    },
    silent: true,
});

and sample.js is:

'use strict';
const process = require('process');

and this results in the error: SyntaxError: Identifier 'process' has already been declared

@ide
Copy link
Contributor Author

ide commented Jan 6, 2017

A quick fix that wouldn't require updating too much code would be to change Electron's module wrapper from:

// start
'(function (exports, require, module, __filename, __dirname, process, global) { '
// end
'}'

to:

// start
'(function (exports, require, module, __filename, __dirname, process, global) {' +
'return (function (exports, require, module, __filename, __dirname) {'
// end
'})(exports, require, module, __filename, __dirname);' +
'}'

This would allow process and global to be shadowed in the module function's scope, while still preserving the semantics for the other injected variables.

@kevinsawicki
Copy link
Contributor

A quick fix that wouldn't require updating too much code would be to change Electron's module wrapper from:

This seems like a good approach to try, would you be up for creating a pull request for this?

@ide
Copy link
Contributor Author

ide commented Jan 9, 2017

@kevinsawicki Sure, is it alright that the PR would touch a file under the vendored copy of Node?

@kevinsawicki
Copy link
Contributor

Sure, is it alright that the PR would touch a file under the vendored copy of Node?

Yeah, you'll just want to fork the node submodule in vendor/node and make your changes in your fork.

ide added a commit to ide/electron that referenced this issue Jan 11, 2017
See the fixed issue for the context. This pulls in a vendored copy of Node that includes the described patch.

Fixes electron#8358

Test Plan: Built Electron and verified it loaded the sample app correctly and that the module wrapper is the new one by viewing Node's source code in the Blink Inspector.
ide added a commit to ide/electron that referenced this issue Jan 19, 2017
See the fixed issue for the context. This pulls in a vendored copy of Node that includes the described patch.

Fixes electron#8358

Test Plan: Built Electron and verified it loaded the sample app correctly and that the module wrapper is the new one by viewing Node's source code in the Blink Inspector.

Added specs and tested with `npm test -- --grep "global variables"`
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 a pull request may close this issue.

2 participants