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

globalThis shim #16054

Closed
wants to merge 2 commits into from
Closed

globalThis shim #16054

wants to merge 2 commits into from

Conversation

bung87
Copy link
Collaborator

@bung87 bung87 commented Nov 19, 2020

Can't assuming users using no-ie and node version >= 12.

@ringabout
Copy link
Member

For reference:
globalThis could be used in most of browsers(91.23%) except IE.
https://caniuse.com/?search=globalThis

And globalThis is introduced to nodejs v11 which is not LTS version and V10 won't be maintained after 2021-04-30

https://nodejs.org/en/about/releases/

@bung87
Copy link
Collaborator Author

bung87 commented Nov 20, 2020

web developers dont want this tiny change breaking their code. company dont want lose their clients.

@@ -2598,6 +2598,13 @@ proc genHeader(): Rope =
var framePtr = null;
var excHandler = 0;
var lastJSError = null;
var globalThis;
if(typeof globalThis == 'undefined') globalThis = (function () {
Copy link
Member

Choose a reason for hiding this comment

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

When would this not be true? You start with var globalThis;

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

var a; typeof a == "undefined";

Copy link
Member

Choose a reason for hiding this comment

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

Exactly, you just wrote if (true). I think in order to emulate globalThis you must not predeclare it at all.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

a= 1; var a; typeof a == "undefined"; it's not like you think.

Copy link
Collaborator Author

@bung87 bung87 Nov 20, 2020

Choose a reason for hiding this comment

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

declare it here just note it as global variable, this line can be safely delete.

@ringabout
Copy link
Member

ringabout commented Nov 20, 2020

Related docs about globalThis
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/globalThis
code from https://github.com/paulmillr/es6-shim/blob/master/es6-shim.js

  var getGlobal = function () {
    /* global self, window */
    // the only reliable means to get the global object is
    // `Function('return this')()`
    // However, this causes CSP violations in Chrome apps.
    if (typeof self !== 'undefined') { return self; }
    if (typeof window !== 'undefined') { return window; }
    if (typeof global !== 'undefined') { return global; }
    throw new Error('unable to locate global object');
  };

@metagn
Copy link
Collaborator

metagn commented Nov 20, 2020

As far as I can tell, Nim only uses globalThis when generating {.global.} variables. I don't think this polyfill needs to be in every single Nim JS output. Why can't we have templates/macros/whatever that emit polyfills when people need them? Those Int*Array definitions are pretty jarring already, it's even worse that "hello world" is now above a kilobyte

@ringabout
Copy link
Member

ringabout commented Nov 20, 2020

As far as I can tell, Nim only uses globalThis when generating {.global.} variables. I don't think this polyfill needs to be in every single Nim JS output. Why can't we have templates/macros/whatever that emit polyfills when people need them? Those Int*Array definitions are pretty jarring already.

Unfortunately, there is one or more variables labeled as threadsafe in system module(namely localRaiseHook_33557682). So globalThis will be shipped with Nim v1.6.0 (but I think it is acceptable). If people need old JS stuff, just stick with old Nim versions(for example Nim v 1.4 ). Since most of browsers and nodejs versions(and V10 won't be maintained after 2021-04-30), it may be unnecessary(increase a bit more code sizes).

@ringabout
Copy link
Member

ringabout commented Nov 20, 2020

Unfortunately, there is one or more variables labeled as threadsafe in system module(namely localRaiseHook_33557682).

I will make a PR to undefine some stuff in JS backend(which is unnecessary for JS backend). Then I guess globalThis won't be shipped with the generated JS code.

Edited:
See PR #16070

ringabout added a commit to ringabout/Nim that referenced this pull request Nov 20, 2020
@ringabout
Copy link
Member

ringabout commented Nov 21, 2020

Those Int*Array definitions are pretty jarring already.

99.22% browsers support Typed Arrays, it's the time to remove them all.

https://caniuse.com/?search=Typed%20Arrays
image

I will make a PR, at least we could remove them eventually in the future.

ringabout added a commit to ringabout/Nim that referenced this pull request Nov 21, 2020
Araq pushed a commit that referenced this pull request Nov 24, 2020
narimiran pushed a commit that referenced this pull request Nov 25, 2020
(cherry picked from commit 3fed854)
ringabout added a commit to ringabout/Nim that referenced this pull request Nov 25, 2020
Araq pushed a commit that referenced this pull request Nov 26, 2020
narimiran pushed a commit that referenced this pull request Nov 26, 2020
mildred pushed a commit to mildred/Nim that referenced this pull request Jan 11, 2021
mildred pushed a commit to mildred/Nim that referenced this pull request Jan 11, 2021
@timotheecour
Copy link
Member

we should instead have a module std/jspolyfills that can be optionally imported by users, instead of defining it in jsgen

there are more use cases of polyfills besides globalThis

links

ardek66 pushed a commit to ardek66/Nim that referenced this pull request Mar 26, 2021
ardek66 pushed a commit to ardek66/Nim that referenced this pull request Mar 26, 2021
@Araq Araq closed this Mar 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants