Skip to content

Use globalThis in embind#8904

Merged
kripken merged 1 commit intoincomingfrom
globalThis
Jul 2, 2019
Merged

Use globalThis in embind#8904
kripken merged 1 commit intoincomingfrom
globalThis

Conversation

@kripken
Copy link
Member

@kripken kripken commented Jul 2, 2019

As reported on the mailing list, this is much faster in some VMs and also has other benefits,

https://groups.google.com/d/msg/emscripten-discuss/6hnm_tJJ018/irC5BLEvCAAJ

$emval_get_global: function() {
if (typeof globalThis === 'object') {
return globalThis;
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When was this introduced? Can we assume globalThis is we are building for wasm and ifdef out the fallback case perhaps?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this meaning this usage of new Function and return this? I think it's part of core embind, so pretty old I imagine.

I don't think we can assume globalThis if wasm is present. E.g. node.js 8.9.1 (emsdk default) has wasm but not globalThis, which appears to be fairly new.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

globalThis is available in V8 v7.1 / Chrome 71, Firefox 65, Safari 12.1, and Node.js v12.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks mathias. I guess that means as soon node v12 or above is LTS we can remove the legacy code.

We could consider depending on a more recent version of node than LTS? I guess we have two uses of node in emscripten:

  1. Emscripten developers who need to run the test suite
  2. Node users who want to put wasm in their node modules.

For (1) we can choose whatever version of node we want. But for (2) its probably good to have continue to support LTS I guess?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that following LTS makes sense for use case 2.

It would be useful to codify the supported Node.js version in the repository by adding an .nvmrc file to the root, and a notice to the readme.

@kripken kripken merged commit e000497 into incoming Jul 2, 2019
@kripken kripken deleted the globalThis branch July 2, 2019 20:45
belraquib pushed a commit to belraquib/emscripten that referenced this pull request Dec 23, 2020
As reported on the mailing list, this is much faster in some VMs and also has other benefits,

https://groups.google.com/d/msg/emscripten-discuss/6hnm_tJJ018/irC5BLEvCAAJ
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.

3 participants