fix(compiler): use either summary or metadata information when reading from `.d.ts` #18912

tbosch · 2017-08-28T22:51:04Z

PR Checklist

Please check if your PR fulfills the following requirements:

The commit message follows our guidelines: https://github.com/angular/angular/blob/master/CONTRIBUTING.md#commit
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

[ ] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] angular.io application / infrastructure changes
[ ] Other... Please describe:

What is the current behavior?

Issue Number: N/A

What is the new behavior?

Does this PR introduce a breaking change?

[ ] Yes
[ ] No

Other information

mary-poppins · 2017-08-28T22:58:58Z

You can preview c5a4e51 at https://pr18912-c5a4e51.ngbuilds.io/.

mary-poppins · 2017-08-29T18:39:05Z

You can preview 81b2ef5 at https://pr18912-81b2ef5.ngbuilds.io/.

chuckjaz

Some questions.

chuckjaz · 2017-08-29T19:02:58Z

packages/compiler/src/aot/static_symbol_resolver.ts

    this.resolvedSymbols.forEach((resolvedSymbol) => {
      if (resolvedSymbol.symbol.filePath === filePath) {
-        symbols.add(resolvedSymbol.symbol);
+        metadataSymbols.push(resolvedSymbol.symbol);


The use of the set was to ensure this array has no duplicates. Is that now ensured some other way?

We needed the Set as we merged 2 list of symbols. As we are now always using either or, and we know that these lists don't contain duplicates, we are good.

chuckjaz · 2017-08-29T19:08:22Z

packages/compiler/src/aot/summary_resolver.ts

    }
-    this.loadedFilePaths.add(filePath);


With this moving to after the if could potentially allow indirect recursion that was prevented by seeding the cache. Is this is concern?

Good point. Will change that.

(although this shouldn't happen...)

mary-poppins · 2017-08-29T20:50:14Z

You can preview 9d38d7d at https://pr18912-9d38d7d.ngbuilds.io/.

tbosch · 2017-08-29T22:56:46Z

Travis is red because of a flake in AIO.

mary-poppins · 2017-08-30T21:35:22Z

You can preview d0218a9 at https://pr18912-d0218a9.ngbuilds.io/.

alexeagle

last two commits

alexeagle · 2017-08-30T23:12:02Z

packages/bazel/src/ngc-wrapped/index.ts

+    throw new Error('rootDirs is not set!');
+  }
+  // When we canonicalize, we want to strip the longest rootDir.
+  rootDirs.sort((a, b) => b.length - a.length);


I think it belongs in rules_typescript, let me move it there?

actually this already happens in the compiler_host.ts from rules_typescript, shouldn't need it

alexeagle · 2017-08-30T23:12:56Z

packages/bazel/src/ngc-wrapped/index.ts

  function relativeToRootDir(filePath: string): string {
    if (tsOptions.rootDir) {
      const rel = path.relative(tsOptions.rootDir, filePath);
      if (rel.indexOf('.') != 0) return rel;
    }
    return filePath;
  }
+
+  function relativeToRootDirs(filePath: string): string {


let's have one function that takes a parameter
roots
that defaults to [tsOptions.rootDir]
to reduce the duplication?

…g .d.ts files This fix applies for getting all symbols in file.

This is a corner case, and converting them is what was expected in G3. This also fits the fact that we already convert package paths into relative paths.

mary-poppins · 2017-08-30T23:19:51Z

You can preview 0eddfdb at https://pr18912-0eddfdb.ngbuilds.io/.

mary-poppins · 2017-08-30T23:26:32Z

You can preview 3dc2c07 at https://pr18912-3dc2c07.ngbuilds.io/.

mary-poppins · 2017-08-30T23:43:30Z

You can preview c3740c3 at https://pr18912-c3740c3.ngbuilds.io/.

…x paths

mary-poppins · 2017-08-31T00:09:05Z

You can preview 63ebff6 at https://pr18912-63ebff6.ngbuilds.io/.

) PR Close #18912

This is a corner case, and converting them is what was expected in G3. This also fits the fact that we already convert package paths into relative paths. PR Close #18912

…x paths (#18912) PR Close #18912

PR Close #18912

angular-automatic-lock-bot · 2019-09-12T09:20:41Z

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.}

googlebot added the cla: yes label Aug 28, 2017

tbosch changed the title ~~Fix g3 2~~ WIP: Fix g3 2 Aug 28, 2017

tbosch added the state: WIP label Aug 28, 2017

tbosch force-pushed the fix_g3_2 branch from c5a4e51 to 81b2ef5 Compare August 29, 2017 18:30

tbosch changed the title ~~WIP: Fix g3 2~~ fix(compiler): use either summary or metadata information when reading from .d.ts Aug 29, 2017

tbosch requested a review from chuckjaz August 29, 2017 18:31

tbosch added area: core Issues related to the framework runtime target: major This PR is targeted for the next major release and removed state: WIP labels Aug 29, 2017

chuckjaz reviewed Aug 29, 2017

View reviewed changes

tbosch force-pushed the fix_g3_2 branch from 81b2ef5 to 9d38d7d Compare August 29, 2017 20:37

chuckjaz approved these changes Aug 29, 2017

View reviewed changes

tbosch added the action: merge The PR is ready for merge by the caretaker label Aug 29, 2017

tbosch removed the action: merge The PR is ready for merge by the caretaker label Aug 30, 2017

tbosch force-pushed the fix_g3_2 branch from d0218a9 to 0eddfdb Compare August 30, 2017 23:10

alexeagle approved these changes Aug 30, 2017

View reviewed changes

tbosch added 3 commits August 30, 2017 16:13

fix(compiler): use either summary or metadata information when readin…

0bcbe62

…g .d.ts files This fix applies for getting all symbols in file.

fix(compiler): workaround bugs in TS when combining transformers

1f2856e

fix(compiler): treat absolute imports as package imports

2d08669

This is a corner case, and converting them is what was expected in G3. This also fits the fact that we already convert package paths into relative paths.

tbosch force-pushed the fix_g3_2 branch from 0eddfdb to 3dc2c07 Compare August 30, 2017 23:13

tbosch force-pushed the fix_g3_2 branch from 3dc2c07 to c3740c3 Compare August 30, 2017 23:30

tbosch force-pushed the fix_g3_2 branch from c3740c3 to 63ebff6 Compare August 30, 2017 23:51

tbosch added 2 commits August 30, 2017 16:52

build: for bazel, paths in summaries should never contain blaze prefi…

3099b3a

…x paths

build: correctly collect summary files with bazel

63ebff6

tbosch added the action: merge The PR is ready for merge by the caretaker label Aug 31, 2017

tbosch mentioned this pull request Aug 31, 2017

Futher fixes for G3 #18986

Closed

jasonaden pushed a commit that referenced this pull request Aug 31, 2017

fix(compiler): workaround bugs in TS when combining transformers (#18912

4059a72

) PR Close #18912

jasonaden pushed a commit that referenced this pull request Aug 31, 2017

build: for bazel, paths in summaries should never contain blaze prefi…

667473c

…x paths (#18912) PR Close #18912

jasonaden pushed a commit that referenced this pull request Aug 31, 2017

build: correctly collect summary files with bazel (#18912)

a1293b2

PR Close #18912

jasonaden closed this in f1e526f Aug 31, 2017

tbosch deleted the fix_g3_2 branch August 31, 2017 21:53

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 12, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(compiler): use either summary or metadata information when reading from `.d.ts` #18912

fix(compiler): use either summary or metadata information when reading from `.d.ts` #18912

tbosch commented Aug 28, 2017

mary-poppins commented Aug 28, 2017

mary-poppins commented Aug 29, 2017

chuckjaz left a comment

chuckjaz Aug 29, 2017

tbosch Aug 29, 2017

chuckjaz Aug 29, 2017

tbosch Aug 29, 2017

tbosch Aug 29, 2017

mary-poppins commented Aug 29, 2017

tbosch commented Aug 29, 2017

mary-poppins commented Aug 30, 2017

alexeagle left a comment

alexeagle Aug 30, 2017

alexeagle Aug 30, 2017

alexeagle Aug 30, 2017

mary-poppins commented Aug 30, 2017

mary-poppins commented Aug 30, 2017

mary-poppins commented Aug 30, 2017

mary-poppins commented Aug 31, 2017

angular-automatic-lock-bot bot commented Sep 12, 2019

fix(compiler): use either summary or metadata information when reading from .d.ts #18912

fix(compiler): use either summary or metadata information when reading from .d.ts #18912

Conversation

tbosch commented Aug 28, 2017

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

mary-poppins commented Aug 28, 2017

mary-poppins commented Aug 29, 2017

chuckjaz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mary-poppins commented Aug 29, 2017

tbosch commented Aug 29, 2017

mary-poppins commented Aug 30, 2017

alexeagle left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mary-poppins commented Aug 30, 2017

mary-poppins commented Aug 30, 2017

mary-poppins commented Aug 30, 2017

mary-poppins commented Aug 31, 2017

angular-automatic-lock-bot bot commented Sep 12, 2019

fix(compiler): use either summary or metadata information when reading from `.d.ts` #18912

fix(compiler): use either summary or metadata information when reading from `.d.ts` #18912