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 global type instances are available. #1175

Merged
merged 1 commit into from
Nov 9, 2018

Conversation

kitsonk
Copy link
Contributor

@kitsonk kitsonk commented Nov 8, 2018

Fixes #1173

It also cleans up a few things related to the globals.ts and provides more comments to help guide additions to the file.

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.

Thanks for the fix ! Can you also add the test case I gave in the issue

@kitsonk kitsonk force-pushed the global-instance-types branch from d09539a to 3d075cb Compare November 9, 2018 00:09
@kitsonk
Copy link
Contributor Author

kitsonk commented Nov 9, 2018

@ry done

import { window } from "./globals";
import { globalEval } from "./global_eval";

const window = globalEval("this");
Copy link
Member

Choose a reason for hiding this comment

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

doing that won't cause window to miss all properties assigned to it in globals.ts? I think we would like to use most of them in REPL

Copy link
Member

Choose a reason for hiding this comment

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

Valid question ... However, I'm going to land this anyway because I need the fix and because this is passing the tests. If this indeed breaks window properties, we should add tests that nail that behavior down.

Copy link
Member

Choose a reason for hiding this comment

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

It doesn't just rebased it against my REPL branch and everything works as expected.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At compile/bundle time this ends up being an any, even when you do import { window } from "./globals.ts"; (because that reference is the same globalEval("this");. At runtime we build the type library which describes what the global scope is, and the ability to access the globalEval module isn't possible. I had previously thought about tightening compile time typings of globalEval("this"); which could be done, but it would be difficult and not really provide any safety, as most of what we do with it is cast it to any anyways.

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

@ry ry merged commit 34b6b86 into denoland:master Nov 9, 2018
ry added a commit to ry/deno that referenced this pull request Nov 12, 2018
- Update to TypeScript 3.1.6 (denoland#1177)
- Fixes Headers type not available. (denoland#1175)
- Reader/Writer to use Uint8Array not ArrayBufferView (denoland#1171)
- Fixes importing modules starting with 'http'. (denoland#1167)
- build: Use target/ instead of out/ (denoland#1153)
- Support repl multiline input (denoland#1165)
@ry ry mentioned this pull request Nov 12, 2018
ry added a commit that referenced this pull request Nov 12, 2018
- Update to TypeScript 3.1.6 (#1177)
- Fixes Headers type not available. (#1175)
- Reader/Writer to use Uint8Array not ArrayBufferView (#1171)
- Fixes importing modules starting with 'http'. (#1167)
- build: Use target/ instead of out/ (#1153)
- Support repl multiline input (#1165)
@kitsonk kitsonk deleted the global-instance-types branch August 2, 2022 04:43
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