NotificationsYou must be signed in to change notification settings
Fork13.2k
Star107k

initial revision of external module augmentations#6213

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

vladima merged 10 commits intomasterfrommoduleAugmentations

Jan 16, 2016

Merged

initial revision of external module augmentations#6213

vladima merged 10 commits intomasterfrommoduleAugmentations

Jan 16, 2016

+7,664 −130

Conversation

Copy link

Contributor

vladima commentedDec 23, 2015

Module augmentation is a declaration of ambient module that directly nested either in external module or in top level ambient external module.
Name of module augmentation is resolved using the same set of rules as module specifiers in import \ export declarations.
If name is successfully resolved to some external module then declarations in module augmentation are merged with declarations inside module using standard rules for declaration merging.
Module augmentations cannot add new items to the top level scope but rather patch existing declarations.

for example

// observable.tsexportclassObservable<T>{}

// map.tsimport{Observable}from"./observable";Observable.prototype.map=function(){/* ... */}declare module"./observable"{interfaceObservable<T>{map<U>(proj:(el:T)=>U):Observable<U>;}}

// consumer.tsimport {Observable} from"./observable";import"./map";let o: Observable<number>;o.map(x=> x.toFixed());

Here modulemap can declare that internally it will patchObservable type and addmap function to it.

Fixes:#5269,#6022

Pending work:

- add syntactic support for augmenting global scope. Currently augmentation for the global scope is defined as ambient module with special name"/" but this is just a placeholder. The version that we ended up during the design meeting was:

// in external moduleexport{};declare global{}

// in script filedeclare module"array"{global{}}

initial revision of external module augmentations

4a963a2

msftclas added the cla-not-required label

Dec 23, 2015

DanielRosenwasser reviewed

Dec 23, 2015

View reviewed changes

src/compiler/binder.ts Outdated

Copy link

Member

DanielRosenwasserDec 23, 2015

Choose a reason for hiding this comment

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

I don't see this getting used anywhere apart from the later assignment.

Copy link

ContributorAuthor

vladimaDec 23, 2015

Choose a reason for hiding this comment

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

right, forgot to remove that. Thanks

address PR feedback

3e1bc01

DanielRosenwasser reviewed

Dec 23, 2015

View reviewed changes

src/compiler/checker.ts Outdated

Copy link

Member

DanielRosenwasserDec 23, 2015

Choose a reason for hiding this comment

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

isGlobalAugmentation or augmentsGlobal

Copy link

Member

DanielRosenwasser commentedDec 23, 2015

What exactly is the recommended way to augment a default export? Is it possible in this implementation?

Copy link

ContributorAuthor

vladima commentedDec 23, 2015

it is not possible

use 'declare global' to define augmentations for the global scope

7f2ebf9

DanielRosenwasser mentioned this pull request

Dec 29, 2015

Reopening external modules broken in TS 1.5-alpha#2784

Closed

Copy link

Member

DanielRosenwasser commentedDec 29, 2015

This also appears to take care of#2784. You might want to take note and give a heads up on that issue when this goes in.

vladima added2 commits

January 11, 2016 22:12

merge with origin/master

7cef1ef

fix linter issues

0e1c6e3

mhegazy reviewed

Jan 13, 2016

View reviewed changes

src/compiler/declarationEmitter.ts

Copy link

Contributor

mhegazyJan 13, 2016

Choose a reason for hiding this comment

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

what about external modules with no exports or no module augmentation? soemthing like:

import{a}from"./mod";// do something with a

Copy link

Contributor

mhegazyJan 13, 2016

Choose a reason for hiding this comment

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

i thought we will do this for any external module, that we never emitted any import, export, or module augmentation for.

address PR feedback

1e11a55

Copy link

Contributor

mhegazy commentedJan 13, 2016

👍

ahejlsberg reviewed

Jan 14, 2016

View reviewed changes

src/compiler/checker.ts Outdated

Copy link

Member

ahejlsbergJan 14, 2016

Choose a reason for hiding this comment

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

This should becheckModuleAugmentationElement. It checks and element in the body, not the body itself.

Copy link

ContributorAuthor

vladimaJan 14, 2016

Choose a reason for hiding this comment

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

✅

addressed PR feedback

3aa92f5

Copy link

ContributorAuthor

vladima commentedJan 14, 2016

build failure is related to#6478

Merge remote-tracking branch 'origin/master' into moduleAugmentations

a69dcd1

Copy link

ContributorAuthor

vladima commentedJan 14, 2016

@ahejlsberg done, do you have any other comments?

merge with origin/master

e90db46

ahejlsberg reviewed

Jan 14, 2016

View reviewed changes

src/compiler/checker.ts

Copy link

Member

ahejlsbergJan 14, 2016

Choose a reason for hiding this comment

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

The error message below seems a bit odd for this case, but maybe it's fine.

Copy link

ContributorAuthor

vladimaJan 14, 2016

Choose a reason for hiding this comment

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

for global case we can easily lift this restriction since it is possible to add new entries to global scope from within a module. Currently the fact that we have it is only because of consistency in behavior for augmentations.

Copy link

Member

ahejlsbergJan 14, 2016

Choose a reason for hiding this comment

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

No sure I understand, maybe we're talking about different issues. This code is saying that you get an error when you have an exported member in a global augmentation block, right? I'm thinking it should never be valid to useexport in a global augmentation block.

Copy link

ContributorAuthor

vladimaJan 14, 2016

Choose a reason for hiding this comment

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

the idea of this check is following:symbol.parent will beundefined if symbol was initially defined in the global scope. Ifsymbol.parent is notundefined this means that this symbol was declared inside augmentation and will declare new entry in the global scope which is disallowed in current design

Copy link

Member

ahejlsbergJan 14, 2016

Choose a reason for hiding this comment

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

If symbol.parent is not undefined this means that this symbol was declared inside augmentation

Why is that so?symbol.parent is non-undefined when the symbol is exported, but it might still be declared inside the augmentation. I'm not getting this.

Copy link

ContributorAuthor

vladimaJan 15, 2016

Choose a reason for hiding this comment

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

symbols whose initial declaration is in global scope are never exported andsymbol.parent for them is alwaysundefined. ifsymbol.parent !== undefined this means that this symbol is exported from somewhere and as a consequence initially defined in augmentation not in global scope

Copy link

Contributor

masaeedu commentedJan 14, 2016

Inmap.ts: since the ambient module declaration advertises to the compiler thatObservable<T> has amap member, would you be able to attach a function of the appropriate kind to the prototype without casting to<any>?

ahejlsberg reviewed

Jan 14, 2016

View reviewed changes

src/compiler/checker.ts Outdated

Copy link

Member

ahejlsbergJan 14, 2016

Choose a reason for hiding this comment

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

I have a hard time reasoning about this.

Copy link

ContributorAuthor

vladimaJan 14, 2016

Choose a reason for hiding this comment

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

this assert is redundant. it basically says: augmentation can only change exported symbol (parent !== undefeind) that really has a declaration (not a virtual symbol likeprototype) - this should always be the case. I'll remove it

Copy link

Member

ahejlsbergJan 14, 2016

Choose a reason for hiding this comment

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

So did you meansymbol.valueDeclaration !== undefined? You currently havesymbol.parent.valueDeclaration.

Copy link

ContributorAuthor

vladimaJan 14, 2016

Choose a reason for hiding this comment

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

symbol.parent.valueDeclaration - is an augmentation:declare module ....symbol.valueDeclaration is declaration inside the augmentation. Currently we report error if container that this symbol is module augmentation and not a normal external module

kwonoj mentioned this pull request

Jan 14, 2016

Cannot opt-in import operator's Type Definitions from 'rxjs/add/operator/***'ReactiveX/rxjs#1193

Closed

This was referencedJan 14, 2016

Merged Declarations for Classes and Interfaces#3333

Merged

Need to support declaration merging in external modules#2821

Closed

address PR feedback

36c489c

vladima added a commit that referenced this pull request

Jan 16, 2016

Merge pull request#6213from Microsoft/moduleAugmentations

c6b0cf1

initial revision of external module augmentations

vladima merged commitc6b0cf1 intomaster

Jan 16, 2016

vladima deleted the moduleAugmentations branch

January 16, 2016 05:28

vladima mentioned this pull request

Jan 16, 2016

External modules that don't export anything emit empty declarations.#6022

Closed

Copy link

benlesh commentedJan 16, 2016

Awesome! Great stuff. Thank you!

Copy link

Member

DanielRosenwasser commentedJan 16, 2016

🎉 👏 👏 👏 👏 👏 👏 👏

Copy link

Member

weswigham commentedJan 18, 2016

👍

vladima mentioned this pull request

Jan 22, 2016

Provide a way to augment global interfaces from external modules#4166

Closed

This was referencedJan 29, 2016

Merged external modules break protected member access checking#6731

Closed

Re-exports of external modules with side effects are erased from ES5 targets#6835

Closed

aikoven mentioned this pull request

Mar 31, 2016

Add state type to dispatch interfacereduxjs/redux#1537

Merged

jkillian mentioned this pull request

Apr 8, 2016

false negative: no-internal-modulepalantir/tslint#1069

Closed

ktsn mentioned this pull request

Aug 24, 2016

Add typescript definitionvuejs/vue#3509

Closed

4 tasks

david-driscoll mentioned this pull request

Jan 4, 2017

Partial classes#563

Closed

microsoft locked and limited conversation to collaborators

Jun 19, 2018

Labels

None yet

10 participants

Movatterモバイル変換

initial revision of external module augmentations#6213

initial revision of external module augmentations#6213

Conversation

vladima commentedDec 23, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DanielRosenwasser commentedDec 23, 2015

Uh oh!

vladima commentedDec 23, 2015

Uh oh!

DanielRosenwasser commentedDec 29, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhegazy commentedJan 13, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vladima commentedJan 14, 2016

Uh oh!

vladima commentedJan 14, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

masaeedu commentedJan 14, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benlesh commentedJan 16, 2016

Uh oh!

DanielRosenwasser commentedJan 16, 2016

Uh oh!

weswigham commentedJan 18, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants