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

ensure that globals includes process and console #18

Merged
merged 1 commit into from Jul 6, 2019

Conversation

@decentralion
Copy link
Contributor

commented Jul 5, 2019

The code attaches global properties to the sandbox context by iterating
over all the enumerable properties of global. However, in node v10,
console switched to being non-enmuerable. This means that for
users of this library with node>10, any console.logs in evaluated
scripts will fail.

This commit fixes this issue by manually attaching console to the
sandbox (when globals are being used). A test has been added. Prior to
the change to eval.js, the test would pass in node v8 but fail in v10
and v12.

Also, the tests were already failing in v12, because in v12 process
also became non-enumerable. I've applied a similar fix to process to
ensure that it's always available too.

ensure that globals includes process and console
The code attaches global properties to the sandbox context by iterating
over all the enumerable properties of `global`. However, in node v10,
`console` switched [to being non-enmuerable][1]. This means that for
users of this library with node>10, any `console.log`s in evaluated
scripts will fail.

This commit fixes this issue by manually attaching console to the
sandbox (when globals are being used). A test has been added. Prior to
the change to eval.js, the test would pass in node v8 but fail in v10
and v12.

Also, the tests were already failing in v12, because in v12 `process`
also became non-enumerable. I've applied a similar fix to `process` to
ensure that it's always available too.

[1]: nodejs/node#17708

@pierrec pierrec merged commit 5135bfb into pierrec:master Jul 6, 2019

@decentralion

This comment has been minimized.

Copy link
Contributor Author

commented Jul 6, 2019

Thanks for merging this, @pierrec. Can you please cut a new release?

@decentralion

This comment has been minimized.

Copy link
Contributor Author

commented Jul 6, 2019

Ah, I see you already did. Thanks!

decentralion added a commit to decentralion/static-site-generator-webpack-plugin that referenced this pull request Jul 6, 2019

Ensure console is available when evaluating
The `eval` dependency has a bug in node v10 and above, where the
`console` won't be available to scripts, even if globals are supposed to
be available. See: pierrec/node-eval#18. For users of
static-site-generator-webpack-plugin, this means that index scripts
which contain logging statements will not work properly.

The bug is fixed in eval v0.1.4, so we just need to update the
dependency.

Test plan: `yarn test` still passes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.