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

fix(ngcc): dts declaration mapping #34254

Conversation

@petebacondarwin
Copy link
Member

petebacondarwin commented Dec 5, 2019

The naïve matching algorithm we previously used to match declarations in
source files to declarations in typings files was based only on the name
of the thing being declared. This did not handle cases where the declared
item has been exported via an alias. This is a common scenariow when one
of the two file sets (source or typings) has been flattened, while the other
has not.

The new algorithm tries to overcome this by creating two maps of export
name to declaration (i.e. Map<string, ts.Declaration>).
One for the source files and one for the typings files.
It then joins these two together by matching export names, resulting in a
new map that maps source declarations to typings declarations directly
(i.e. Map<ts.Declaration, ts.Declaration>).

This new map can handle the declaration names being different between the
source and typings as long as they are ultimately both exported with the
same alias name.

Fixes #33593
Fixes #34191
Fixes #32442

@googlebot googlebot added the cla: yes label Dec 5, 2019
@petebacondarwin petebacondarwin force-pushed the petebacondarwin:ngcc-dts-declaration-mapping branch 2 times, most recently from 504814c to ddd998b Dec 7, 2019
@petebacondarwin petebacondarwin marked this pull request as ready for review Dec 7, 2019
@petebacondarwin petebacondarwin requested review from angular/fw-compiler as code owners Dec 7, 2019
@ngbot ngbot bot modified the milestone: needsTriage Dec 7, 2019
@petebacondarwin petebacondarwin force-pushed the petebacondarwin:ngcc-dts-declaration-mapping branch from ddd998b to 9c47295 Dec 8, 2019
Copy link
Member

gkalpak left a comment

LGTM for fw-ngcc 👍

@petebacondarwin petebacondarwin force-pushed the petebacondarwin:ngcc-dts-declaration-mapping branch 2 times, most recently from 3d12e8e to aaa1733 Dec 11, 2019
@petebacondarwin petebacondarwin force-pushed the petebacondarwin:ngcc-dts-declaration-mapping branch from aaa1733 to a5ac793 Dec 11, 2019
@petebacondarwin petebacondarwin requested review from angular/dev-infra-framework as code owners Dec 11, 2019
@googlebot

This comment has been minimized.

Copy link

googlebot commented Dec 11, 2019

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added cla: no and removed cla: yes labels Dec 11, 2019
@alxhub

This comment has been minimized.

Copy link
Contributor

alxhub commented Dec 18, 2019

@petebacondarwin petebacondarwin removed the request for review from gkalpak Dec 18, 2019
statement.expression.expression.text === '__export' &&
statement.expression.arguments.length === 1 &&
ts.isIdentifier(statement.expression.arguments[0]);
}

This comment has been minimized.

Copy link
@gkalpak

gkalpak Dec 18, 2019

Member

Can't UMD have __export(require('...')) type re-exports? I thought it could.

This comment has been minimized.

Copy link
@petebacondarwin

petebacondarwin Dec 18, 2019

Author Member

You can have commonjs require imports but they must be passed to the factory function. I dont see how you could have require reexports without breaking non-commonjs formats

This comment has been minimized.

Copy link
@gkalpak

gkalpak Dec 19, 2019

Member

My understanding is that you could also have require(...) calls inside the factory (what RequireJS calls simplified CommonJS wrapping). Also, the TypeScript playground seems to generate that format for UMD (but that might be because the playground doesn't do a proper build): TS playground example

This comment has been minimized.

Copy link
@petebacondarwin

petebacondarwin Dec 19, 2019

Author Member

OK, so AMD supports a require argument passed through to the factory function, so
I guess it is possible and we should perhaps support it... But in that case you cannot use the UMD templates that support globals. See https://github.com/umdjs/umd

This comment has been minimized.

Copy link
@gkalpak

gkalpak Dec 19, 2019

Member

I am not arguing whether it is a good idea or not to use require() calls in UMD, but if it is possible that the UMD files we need to process use that, then it would be better if we detected it.

I'd be happy to add support for this in a PR I am working on to fix star re-exports with imported helpers.

This comment has been minimized.

Copy link
@gkalpak

gkalpak Dec 20, 2019

Member

For reference: #34512

@kara kara closed this in 0b837e2 Dec 18, 2019
kara added a commit that referenced this pull request Dec 18, 2019
In TS we can re-export imports using statements of the form:

```
export * from 'some-import';
```

This can be downleveled in CommonJS to either:

```
__export(require('some-import'));
```

or

```
var someImport = require('some-import');
__export(someImport);
```

Previously we only supported the first downleveled version.
This commit adds support for the second version.

PR Close #34254
kara added a commit that referenced this pull request Dec 18, 2019
In TS we can re-export imports using statements of the form:

```
export * from 'some-import';
```

This is downleveled in UMD to:

```
function factory(exports, someImport) {
  function __export(m) {
    for (var p in m) if (!exports.hasOwnProperty(p)) exports[p] = m[p];
  }
  __export(someImport);
}
```

This commit adds support for this.

PR Close #34254
kara added a commit that referenced this pull request Dec 18, 2019
…34254)

The naïve matching algorithm we previously used to match declarations in
source files to declarations in typings files was based only on the name
of the thing being declared.  This did not handle cases where the declared
item had been exported via an alias - a common scenario when one of the two
file sets (source or typings) has been flattened, while the other has not.

The new algorithm tries to overcome this by creating two maps of export
name to declaration (i.e. `Map<string, ts.Declaration>`).
One for the source files and one for the typings files.
It then joins these two together by matching export names, resulting in a
new map that maps source declarations to typings declarations directly
(i.e. `Map<ts.Declaration, ts.Declaration>`).

This new map can handle the declaration names being different between the
source and typings as long as they are ultimately both exported with the
same alias name.

Further more, there is one map for "public exports", i.e. exported via the
root of the source tree (the entry-point), and another map for "private
exports", which are exported from individual files in the source tree but
not necessarily from the root. This second map can be used to "guess"
the mapping between exports in a deep (non-flat) file tree, which can be
used by ngcc to add required private exports to the entry-point.

Fixes #33593

PR Close #34254
kara added a commit that referenced this pull request Dec 18, 2019
…34254)

Previously the identifiers used in the typings files were the same as
those used in the source files.

When the typings files and the source files do not match exactly, e.g.
when one of them is flattened, while the other is a deep tree, it is
possible for identifiers to be renamed.

This commit ensures that the correct identifier is used in typings files
when the typings file does not export the same name as the source file.

Fixes angular/ngcc-validation#608

PR Close #34254
kara added a commit that referenced this pull request Dec 18, 2019
Now that the source to typings matching is able to handle
aliasing of exports, there is no need to handle aliases in private
declarations analysis.

These were originally added to cope when the typings files had
to use the name that the original source files used when exporting.

PR Close #34254
kara added a commit that referenced this pull request Dec 18, 2019
Previously individual properties of the src bundle program were
passed to the reflection host constructors. But going forward,
more properties will be required. To prevent the signature getting
continually larger and more unwieldy, this change just passes the
whole src bundle to the constructor, allowing it to extract what it
needs.

PR Close #34254
kara added a commit that referenced this pull request Dec 18, 2019
In TS we can re-export imports using statements of the form:

```
export * from 'some-import';
```

This can be downleveled in CommonJS to either:

```
__export(require('some-import'));
```

or

```
var someImport = require('some-import');
__export(someImport);
```

Previously we only supported the first downleveled version.
This commit adds support for the second version.

PR Close #34254
kara added a commit that referenced this pull request Dec 18, 2019
In TS we can re-export imports using statements of the form:

```
export * from 'some-import';
```

This is downleveled in UMD to:

```
function factory(exports, someImport) {
  function __export(m) {
    for (var p in m) if (!exports.hasOwnProperty(p)) exports[p] = m[p];
  }
  __export(someImport);
}
```

This commit adds support for this.

PR Close #34254
kara added a commit that referenced this pull request Dec 18, 2019
…34254)

The naïve matching algorithm we previously used to match declarations in
source files to declarations in typings files was based only on the name
of the thing being declared.  This did not handle cases where the declared
item had been exported via an alias - a common scenario when one of the two
file sets (source or typings) has been flattened, while the other has not.

The new algorithm tries to overcome this by creating two maps of export
name to declaration (i.e. `Map<string, ts.Declaration>`).
One for the source files and one for the typings files.
It then joins these two together by matching export names, resulting in a
new map that maps source declarations to typings declarations directly
(i.e. `Map<ts.Declaration, ts.Declaration>`).

This new map can handle the declaration names being different between the
source and typings as long as they are ultimately both exported with the
same alias name.

Further more, there is one map for "public exports", i.e. exported via the
root of the source tree (the entry-point), and another map for "private
exports", which are exported from individual files in the source tree but
not necessarily from the root. This second map can be used to "guess"
the mapping between exports in a deep (non-flat) file tree, which can be
used by ngcc to add required private exports to the entry-point.

Fixes #33593

PR Close #34254
kara added a commit that referenced this pull request Dec 18, 2019
…34254)

Previously the identifiers used in the typings files were the same as
those used in the source files.

When the typings files and the source files do not match exactly, e.g.
when one of them is flattened, while the other is a deep tree, it is
possible for identifiers to be renamed.

This commit ensures that the correct identifier is used in typings files
when the typings file does not export the same name as the source file.

Fixes angular/ngcc-validation#608

PR Close #34254
kara added a commit that referenced this pull request Dec 18, 2019
Now that the source to typings matching is able to handle
aliasing of exports, there is no need to handle aliases in private
declarations analysis.

These were originally added to cope when the typings files had
to use the name that the original source files used when exporting.

PR Close #34254
@petebacondarwin petebacondarwin deleted the petebacondarwin:ngcc-dts-declaration-mapping branch Dec 19, 2019
gkalpak added a commit to gkalpak/angular that referenced this pull request Dec 20, 2019
This fix was part of a broader `ngtsc`/`ngcc` fix in 02bab8c (see
there for details). In 02bab8c, the fix was only applied to
`CommonJsReflectionHost`, but it is equally applicable to
`UmdReflectionHost`. Later in angular#34254, the fix was partially ported to
`UmdReflectionHost` by fixing the `extractUmdReexports()` method.

This commit fully fixes `ngcc`'s handling of inline exports for code in
UMD format.
gkalpak added a commit to gkalpak/angular that referenced this pull request Dec 20, 2019
This fix was part of a broader `ngtsc`/`ngcc` fix in 02bab8c (see
there for details). In 02bab8c, the fix was only applied to
`CommonJsReflectionHost`, but it is equally applicable to
`UmdReflectionHost`. Later in angular#34254, the fix was partially ported to
`UmdReflectionHost` by fixing the `extractUmdReexports()` method.

This commit fully fixes `ngcc`'s handling of inline exports for code in
UMD format.
gkalpak added a commit to gkalpak/angular that referenced this pull request Dec 29, 2019
This fix was part of a broader `ngtsc`/`ngcc` fix in 02bab8c (see
there for details). In 02bab8c, the fix was only applied to
`CommonJsReflectionHost`, but it is equally applicable to
`UmdReflectionHost`. Later in angular#34254, the fix was partially ported to
`UmdReflectionHost` by fixing the `extractUmdReexports()` method.

This commit fully fixes `ngcc`'s handling of inline exports for code in
UMD format.
gkalpak added a commit to gkalpak/angular that referenced this pull request Jan 8, 2020
This fix was part of a broader `ngtsc`/`ngcc` fix in 02bab8c (see
there for details). In 02bab8c, the fix was only applied to
`CommonJsReflectionHost`, but it is equally applicable to
`UmdReflectionHost`. Later in angular#34254, the fix was partially ported to
`UmdReflectionHost` by fixing the `extractUmdReexports()` method.

This commit fully fixes `ngcc`'s handling of inline exports for code in
UMD format.
alxhub added a commit that referenced this pull request Jan 8, 2020
This fix was part of a broader `ngtsc`/`ngcc` fix in 02bab8c (see
there for details). In 02bab8c, the fix was only applied to
`CommonJsReflectionHost`, but it is equally applicable to
`UmdReflectionHost`. Later in #34254, the fix was partially ported to
`UmdReflectionHost` by fixing the `extractUmdReexports()` method.

This commit fully fixes `ngcc`'s handling of inline exports for code in
UMD format.

PR Close #34512
alxhub added a commit that referenced this pull request Jan 8, 2020
This fix was part of a broader `ngtsc`/`ngcc` fix in 02bab8c (see
there for details). In 02bab8c, the fix was only applied to
`CommonJsReflectionHost`, but it is equally applicable to
`UmdReflectionHost`. Later in #34254, the fix was partially ported to
`UmdReflectionHost` by fixing the `extractUmdReexports()` method.

This commit fully fixes `ngcc`'s handling of inline exports for code in
UMD format.

PR Close #34512
@angular-automatic-lock-bot

This comment has been minimized.

Copy link

angular-automatic-lock-bot bot commented Jan 20, 2020

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Jan 20, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
4 participants
You can’t perform that action at this time.