-
Notifications
You must be signed in to change notification settings - Fork 12.6k
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
Add globalThis #29332
Add globalThis #29332
Conversation
With one or two additional comments
Still need to 1. Make it work in Typescript. 2. Add test (and make them work) for the other uses of GlobalThis: window, globalThis, etc.
Lots of tests still fail, but all but 1 change so far has been correct.
A couple of tests still fail and need to be fixed.
The type reference must be `typeof globalThis`. Just `globalThis` will be treated as a value reference in type position -- an error.
I left the noImplicitThis rule for captured use of global this in an arrow function, even though technically it isn't `any` any more -- it's typeof globalThis. However, you should still use some other method to access globals inside an arrow, because captured-global-this is super confusing there.
I ran into a problem with intersecting `Window & typeof globalThis`: 1. This adds a new index signature to Window, which is probably not desired. In fact, with noImplicitAny, it's not desired on globalThis either I think. 2. Adding this type requires editing TSJS-lib-generator, not this repo. So I added the test cases and will probably update them later, when those two problems are fixed.
I decided I didn't like the import-type-based approach. Update baselines to reflect the difference.
|
I assume this is specifically about sloppy mode code, right? In strict mode, or in modules, the global |
From later discussion, I noticed that |
@Kovensky you are right but that would probably be a big breaking change. It'll be easier to test in a separate PR, so I will do it that way. It would be a good idea to ship that change in the same version as this one, though. |
globalThis is no longer constructed lazily. Its synthetic Identifier node is also now more realistic.
@@ -12,4 +12,4 @@ | |||
//// // different 'this' | |||
//// function f(this) { return this; } | |||
|
|||
verify.singleReferenceGroup("this"); | |||
verify.singleReferenceGroup("module globalThis\nthis: typeof globalThis"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm. We might want a custom way to display the symbol associated with the global scope. Like just (global) globalThis
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like the current state, but isn't there some value to printing typeof globalThis
? It teaches people what type they have to write to refer to the type of globalThis.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
eh? I don't really feel too strongly about it. It just felt like for something as core as a reference to the global scope, something a little more clear then module globalThis; this: typeof globalThis
may be warranted. It's a unique object and so maybe deserves unique output.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't expect, and don't like, module globalThis
. I'll figure out why it appears.
On the other hand, globalThis
is a unique object, but one whose binding acts like any other global variable, so it weirds me out a little to say that this
should display as globalThis
at top-level. I kind of like saying that this has the type typeof globalThis
instead.
Let's discuss at the design meeting. I could go either way.
I just ran the user tests and nothing failed there. That's a good sign. |
Notes from Design meeting:
|
In progress, had to interrupt for other work.
Is this the same as what That is, the outerWobbler variable in namespace PluginA can now be defined of type IWobbler with the help of globalThis below?
Workaround at the moment is something like
|
@IanYates This is already available as This PR adds a type for the global namespace so you get that instead of |
All right, I think this is ready to go. @weswigham Mind taking another look? |
1. Add parameter to tryGetThisTypeAt to exclude globalThis. 2. Use combined Module flag instead combining them in-place. 3. SymbolDisplay doesn't print 'module globalThis' for this expressions anymore.
if (outsideThis) { | ||
addRelatedInfo(diag, createDiagnosticForNode(container, Diagnostics.An_outer_value_of_this_is_shadowed_by_this_container)); | ||
const type = tryGetThisTypeAt(node, /*includeGlobalThis*/ true, container); | ||
if (noImplicitThis) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than switching on noImplicitThis
out here, shouldn't we use errorOrSuggestion
(switching on noImplicitThis) instead of error
so we get suggestions for these issues even when noImplicitThis
is off?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestions are only used for triggering codefixes, I think. And there aren't any codefixes for these errors. If both of these are true, then let's just wait until we have a codefix for them.
And I can't think of a good codefix for any of the errors except perhaps "The containing arrow function captures the global value of this", which would convert the arrow function to a function expression.
@@ -19343,6 +19362,12 @@ namespace ts { | |||
if (isJSLiteralType(leftType)) { | |||
return anyType; | |||
} | |||
if (leftType.symbol === globalThisSymbol) { | |||
if (noImplicitAny) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, but for noImplicitAny
.
Apparently the current implementation only works when variables are defined in a global file. Currently, I'm using declare global {
interface Window {
MyAppStores: {
FileStore: FileStoreI;
}
}
}
...
window.MyAppStore = { FileStore } |
@dperetti you can try this interface AppStore {
FileStore: FileStoreI;
}
declare global {
var MyAppStores: AppStore;
} Both |
It doesn't seem there is a good way to extend interface globalThis extends Window {} The above keeps failing with:
I'm on typescript version 4.1. Also I was expecting that extending interface RuntimeGlobals {}
interface Window extends RuntimeGlobals {} |
globalThis now has a name and has advanced to stage 3, so this PR adds it to Typescript. It's based on #22891, which we delayed until the proposal had come up with a name.
This change injects a global namespace symbol
globalThis
whose exports is the global symbol table. This enables much better checking.If you need to refer to the global type, use
typeof globalThis
.Open items:
window
should have typeWindow & typeof globalThis
, and window property assignments (window.x = 1
) should add to globals, at least at top-level, or when window is otherwise still bound to the global object. This is probably a big breaking change (in TSJS-lib-generator no less), and should be tested separately.this: undefined
is correct, notthis: typeof globalThis
. However, this is probably a big breaking change. It should be tested separately.Notes:
globalThis
is always available, no matter what the target emit is. I'll add code to downlevelglobalThis
tothis
(or a helper) in a future PR.globalThis
is a namespace,typeof globalThis
doesn't have an index signature even though Typescript currently allows arbitrary property and element accesses on globalthis
. I added special-case code in checkPropertyAccessExpression to allow arbitrary property accesses, and it gives an error whennoImplicitAny
is on.this
. Strictly speakingthis
isn't of typeany
, buttypeof globalThis
, but it's still not good code; people should useglobalThis.x
instead ofthis.x
because it's less confusing.this.x = 12
property assignments as an equivalent declaration tovar x = 12
at the global scope in JS. This code is based on the previous PR Bind toplevel this assignments as global declarations #22891.