Repository navigation
Skip ambient modules in globalThis - #48938
Conversation
Previously, globalThis mistakenly included ambient modules, even though
these are not values:
```ts
declare module "ambientModule" {
export type typ = 1
export var val: typ
}
type Oops = (typeof globalThis)[\"ambientModule\"]
```
This PR adds ambient modules to the kinds of things that are skipped
when constructing `globalThis`' properties, along with block-scoped
variables.
TypeScript Bot (typescript-bot)
added
Author: Team
For Milestone Bug
| const varsOnly = new Map<string, Symbol>() as SymbolTable; | ||
| members.forEach(p => { | ||
| if (!(p.flags & SymbolFlags.BlockScoped)) { | ||
| if (!(p.flags & SymbolFlags.BlockScoped) && !(p.flags & SymbolFlags.ValueModule && some(p.declarations, isAmbientModule))) { |
There was a problem hiding this comment.
some questions:
- what's this
ValueModulesymbol flag/what is a value module? - is it precise enough to check if some declaration is an ambient module? what if some of the other declarations are not ambient modules? (that question may be answered by the one above)
There was a problem hiding this comment.
- A value module is a namespace OR
declare module "foo"with values inside it. A namespace module is a namespace that has only types inside it. (Both kinds of module can contain other namespaces; any nested value turns the parent namespaces into value modules.) - I think the binder only allows ambient modules to merge with ambient modules? I'll have to check.
There was a problem hiding this comment.
Hm, the excludes are the same as other value modules: Value & ~(Function | Class | RegularEnum | ValueModule)
I'll see if I can name a function, class or enum "foo", but I don't think I can.
There was a problem hiding this comment.
So, effectively only other ambient modules can merge with ambient modules.
There was a problem hiding this comment.
I'll see if I can name a function, class or enum "foo", but I don't think I can.
You can if you use a js merge, eg,
globalThis['"module"'] = function () {}or similar.
|
|
||
| type GlobalThis = keyof typeof globalThis | ||
| // should error | ||
| const bad2: GlobalThis = 'ambientModule' |
There was a problem hiding this comment.
I'm not sure I understand what's being tested here, because the use of quotes makes it confusing for me.
const bad2: GlobalThis = 'ambientModule' should error with or without this bug fix, no? Because the ambient module is "ambientModule", with the quotes. Or am I misunderstanding something?
There was a problem hiding this comment.
This was the original test case, which was already confusing. I should probably just leave it out: it turns out that keyof turns "\"foo\"" into just "foo". But it's not relevant to whether "\"foo\"" should be there in the first place.
| const varsOnly = new Map<string, Symbol>() as SymbolTable; | ||
| members.forEach(p => { | ||
| if (!(p.flags & SymbolFlags.BlockScoped)) { | ||
| if (!(p.flags & SymbolFlags.BlockScoped) && !(p.flags & SymbolFlags.ValueModule && some(p.declarations, isAmbientModule))) { |
There was a problem hiding this comment.
some? What if you make a global named "module" that ?merges? with the ambient module named "module"? (Is such a merge even possible?)
There was a problem hiding this comment.
How do you make a global named "module"?
Edit: Ah, you give an example above. I guess every then?
The modules are required to have at least one declaration so that our treatment of `globalThis` stays the same, and `globalThis.globalThis.globalThis` remains legal.
4 participants
Previously, globalThis mistakenly included ambient modules, even though these are not values:
This PR adds ambient modules to the kinds of things that are skipped when constructing
globalThis' properties, along with block-scoped variables.Fixes #48147