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

refactor: remove global_eval.ts #1813

Merged
merged 1 commit into from
Feb 20, 2019
Merged

Conversation

kt3k
Copy link
Member

@kt3k kt3k commented Feb 20, 2019

This PR replaced the globalEval usages with exported window object. I created //js/window.ts file to separate const window declaration from other globals settings and avoid circular reference.

// TODO: this should be replaced with globalThis
// when the proposal goes to stage 4
// See https://github.com/tc39/proposal-global
export const window = (0, eval)("this");
Copy link
Member

Choose a reason for hiding this comment

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

What does this mean? (0, eval) ?

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it's indirect call of eval. Basically the same as the previous code. Please see the stackoverflow link above for details.

Copy link
Member

Choose a reason for hiding this comment

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

Please add a comment.. Something like this

// (0, eval) is indirect eval https://tc39.github.io/ecma262/#sec-performeval

Copy link
Member

@ry ry left a comment

Choose a reason for hiding this comment

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

+16 −35

nice

@kt3k kt3k force-pushed the feature/global-eval branch from aed71ea to efd9fe6 Compare February 20, 2019 02:06
@kt3k
Copy link
Member Author

kt3k commented Feb 20, 2019

Updated the comments. Fixed the lint errors.

Copy link
Member

@ry ry left a comment

Choose a reason for hiding this comment

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

LGTM - thanks

@ry ry merged commit c4e3728 into denoland:master Feb 20, 2019
@kt3k kt3k deleted the feature/global-eval branch February 20, 2019 02:44
Copy link
Contributor

@kitsonk kitsonk left a comment

Choose a reason for hiding this comment

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

We should try to use globalThis now if we are going to refactor it.

// - https://stackoverflow.com/a/14120023
// - https://tc39.github.io/ecma262/#sec-performeval (spec)
export const window = (0, eval)("this");
// TODO: The above should be replaced with globalThis
Copy link
Contributor

Choose a reason for hiding this comment

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

globalThis is already in the runtime... We should be able to do this now, but when I tried it before, there was some sort of error in the snapshotting, I don't know if the snapshot is being done with the right config or some other issue.

@kt3k
Copy link
Member Author

kt3k commented Feb 20, 2019

@kitsonk
I've seen this slide which seems presented in TC39 in last January. It proposes the new name Global for the global object, and the slide seems saying that the champion (Jordan Harband) is still open to that change. So I'm afraid it's still not completely fixed, and it's safe to do it after it reaches stage 4.

@kitsonk
Copy link
Contributor

kitsonk commented Feb 20, 2019

@kt3k from the notes: https://tc39.github.io/tc39-notes/2019-01_jan-31.html#globalthis-follow-up it was decided to keep globalThis and not consider Global. It isn't going to change at this stage because it is already shipped. The conclusion is that the process is broken and the secrecy around the name was not good and it is akin to smooshgate.

TypeScript is just about to implement it into master: microsoft/TypeScript#29332

@kt3k
Copy link
Member Author

kt3k commented Feb 20, 2019

@kitsonk Thank you for the pointer! OK. Let's replace it soon! 👍

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.

4 participants