ts-api-guardian: overloading a function doesn't generate all of the signatures #22569

alan-agius4 · 2018-03-03T11:36:07Z

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?

[x] 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?

Overloaded pure functions are not generated.

Issue Number: angular/ts-api-guardian#22

What is the new behavior?

Overloaded methods are generated

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

Deltas

diff --git a/tools/public_api_guard/common/common.d.ts b/tools/public_api_guard/common/common.d.ts
index 9d07132c2..cebf029d7 100644
--- a/tools/public_api_guard/common/common.d.ts
+++ b/tools/public_api_guard/common/common.d.ts
@@ -306 +306 @@ export declare class NgLocaleLocalization extends NgLocalization {
-        deprecatedPluralFn?: ((locale: string, value: string | number) => Plural) | null | undefined);
+        /** @deprecated */ deprecatedPluralFn?: ((locale: string, value: string | number) => Plural) | null | undefined);
diff --git a/tools/public_api_guard/core/core.d.ts b/tools/public_api_guard/core/core.d.ts
index 835512649..83d605e4e 100644
--- a/tools/public_api_guard/core/core.d.ts
+++ b/tools/public_api_guard/core/core.d.ts
@@ -451,0 +452 @@ export declare function inject<T>(token: Type<T> | InjectionToken<T>, notFoundVa
+export declare function inject<T>(token: Type<T> | InjectionToken<T>, notFoundValue: T | null, flags?: InjectFlags): T | null;
diff --git a/tools/public_api_guard/core/testing.d.ts b/tools/public_api_guard/core/testing.d.ts
index d4c5452ba..b5fcd428c 100644
--- a/tools/public_api_guard/core/testing.d.ts
+++ b/tools/public_api_guard/core/testing.d.ts
@@ -158,0 +159 @@ export declare function withModule(moduleDef: TestModuleMetadata): InjectSetupWr
+export declare function withModule(moduleDef: TestModuleMetadata, fn: Function): () => any;
diff --git a/tools/public_api_guard/router/testing.d.ts b/tools/public_api_guard/router/testing.d.ts
index d14731865..e7877cdc2 100644
--- a/tools/public_api_guard/router/testing.d.ts
+++ b/tools/public_api_guard/router/testing.d.ts
@@ -7,0 +8 @@ export declare function setupTestingRouter(urlSerializer: UrlSerializer, context
+export declare function setupTestingRouter(urlSerializer: UrlSerializer, contexts: ChildrenOutletContexts, location: Location, loader: NgModuleFactoryLoader, compiler: Compiler, injector: Injector, routes: Route[][], urlHandlingStrategy?: UrlHandlingStrategy): Router;

/cc @ocombe

alfaproject · 2018-03-03T12:23:44Z

tools/ts-api-guardian/lib/serializer.ts

+    let children: ts.Node[] = [];
+    if (ts.isFunctionDeclaration(node)) {
+      const symbol = this.typeChecker.getSymbolAtLocation(node.name);
+      symbol.declarations.forEach(x => children.push(...x.getChildren()));


Concat is faster than push/spread if you care: dec => children = children.concat(dec.getChildren())

alan-agius4 · 2018-03-03T15:25:46Z

hey @alexeagle, I am having a bit of a problem here when I am doing changes in the serializer.

Basicilly bazel test is using the local ts-api-guardian, but the gulp public-api:enforce is using the node_modules one.

Bazel fails when golden files are not updated
https://circleci.com/gh/angular/angular/34377?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link

Gulp fails when golden files are updated
https://travis-ci.org/angular/angular/jobs/348657294

So, it's impossible to have a green CI.

alexeagle · 2018-03-05T18:07:00Z

@alan-agius4 that's a good point. In this case, can we just exclude core and common from the check that relies on the npm package? If not, I'll finish converting us over to the locally built one.

alan-agius4 · 2018-03-05T18:25:51Z

Yes, absolutely, I pushed a change

alexeagle · 2018-03-05T21:36:13Z

tools/gulp-tasks/public-api.js

@@ -34,7 +36,8 @@ const entrypoints = [
  'dist/packages-dist/platform-server/platform-server.d.ts',
  'dist/packages-dist/platform-server/testing.d.ts',
  'dist/packages-dist/router/router.d.ts',
-  'dist/packages-dist/router/testing.d.ts',
+  // The below has been excluded until we use the local build of API guardian
+  //'dist/packages-dist/router/testing.d.ts',


We don't build an ng_package for router, so this isn't safe to remove...

I think this means we need to do it now, or else you'll have a test failure on one side or the other due to the router change in this PR?

Yes indeed we so need to migrate to locally built guardian now, as E2E will fail like this.

I actually have an idea of what we can do at least until everything is migrated. (Not sure how much time that involves)

My idea is that when we call gulp public-api:enforce we can a pre-gulp task invoking

bazel run //:install bazel build //tools/ts-api-guardian:lib

And than from the gulp tasks, rather than requiring the node_module we require the local guardian.

alan-agius4 · 2018-03-05T21:39:08Z

tools/ts-api-guardian/lib/serializer.ts

@@ -215,7 +215,14 @@ class ResolvedDeclarationEmitter {
      }
    }

-    let children = node.getChildren();
+    let children: ts.Node[] = [];
+    if (ts.isFunctionDeclaration(node)) {


Used ts.isFunctionDeclaration instead of node.kind because this is a type guard.

I'd put this kind of thing as a comment in the source code - many people will read that later, no one will see this thread

alexeagle · 2018-03-07T18:14:42Z

let's land #22628 first to clean this up. I think you need router to be tested with the local ts-api-guardian.

alexeagle · 2018-03-07T18:57:33Z

okay that's @alan-agius4 that's merged, can you re-base and green this up? thanks!!

ngbot · 2018-03-07T19:13:39Z

Hi @alan-agius4! This PR has merge conflicts due to recent upstream merges.
Please help to unblock it by resolving these conflicts. Thanks!

alan-agius4 · 2018-03-07T19:13:59Z

Yes sure, give me a couple of hrs and I am on it!

…

On Wed, 07 Mar 2018 at 19:58, Alex Eagle ***@***.***> wrote: okay that's @alan-agius4 <https://github.com/alan-agius4> that's merged, can you re-base and green this up? thanks!! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#22569 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AQv-Wqzz9SW7I5Upuc0nxAm65Le_02KSks5tcC3agaJpZM4Sa4tL> .

alan-agius4 · 2018-03-07T20:44:48Z

@alexeagle it’s green 🎉

alexeagle

Awesome thank you!!

alexeagle · 2018-03-09T14:10:59Z

tools/ts-api-guardian/lib/serializer.ts

@@ -215,7 +215,14 @@ class ResolvedDeclarationEmitter {
      }
    }

-    let children = node.getChildren();
+    let children: ts.Node[] = [];
+    if (ts.isFunctionDeclaration(node)) {


I'd put this kind of thing as a comment in the source code - many people will read that later, no one will see this thread

IgorMinar

Thank you so much for this fix!

alan-agius4 · 2018-03-09T19:28:58Z

Welcome, glad to help.

…ngular#22569) PR Close angular#22569

angular-automatic-lock-bot · 2019-09-13T10:21:19Z

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 Mar 3, 2018

alfaproject reviewed Mar 3, 2018

View reviewed changes

alexeagle self-assigned this Mar 5, 2018

alexeagle added the target: major This PR is targeted for the next major release label Mar 5, 2018

alexeagle reviewed Mar 5, 2018

View reviewed changes

alan-agius4 commented Mar 5, 2018

View reviewed changes

alexeagle added the area: build & ci Related the build and CI infrastructure of the project label Mar 5, 2018

alexeagle approved these changes Mar 9, 2018

View reviewed changes

fix: overloading a function doesn't generate all of the signatures

d66e0b7

IgorMinar approved these changes Mar 9, 2018

View reviewed changes

IgorMinar added the action: merge The PR is ready for merge by the caretaker label Mar 9, 2018

kara closed this in e8326e6 Mar 9, 2018

leo6104 pushed a commit to leo6104/angular that referenced this pull request Mar 25, 2018

fix: overloading a function doesn't generate all of the signatures (a…

0a7d8c5

…ngular#22569) PR Close angular#22569

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ts-api-guardian: overloading a function doesn't generate all of the signatures #22569

ts-api-guardian: overloading a function doesn't generate all of the signatures #22569

alan-agius4 commented Mar 3, 2018 •

edited

alfaproject Mar 3, 2018 •

edited

alan-agius4 commented Mar 3, 2018 •

edited

alexeagle commented Mar 5, 2018

alan-agius4 commented Mar 5, 2018

alexeagle Mar 5, 2018

alan-agius4 Mar 5, 2018

alexeagle Mar 5, 2018

alan-agius4 Mar 5, 2018 •

edited

alan-agius4 Mar 6, 2018 •

edited

alan-agius4 Mar 5, 2018

alexeagle Mar 9, 2018

alan-agius4 Mar 9, 2018

alexeagle commented Mar 7, 2018

alexeagle commented Mar 7, 2018

ngbot bot commented Mar 7, 2018

alan-agius4 commented Mar 7, 2018 via email •

edited

alan-agius4 commented Mar 7, 2018

alexeagle left a comment

alexeagle Mar 9, 2018

IgorMinar left a comment

alan-agius4 commented Mar 9, 2018

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

ts-api-guardian: overloading a function doesn't generate all of the signatures #22569

ts-api-guardian: overloading a function doesn't generate all of the signatures #22569

Conversation

alan-agius4 commented Mar 3, 2018 • edited

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

alfaproject Mar 3, 2018 • edited

Choose a reason for hiding this comment

alan-agius4 commented Mar 3, 2018 • edited

alexeagle commented Mar 5, 2018

alan-agius4 commented Mar 5, 2018

alexeagle Mar 5, 2018

Choose a reason for hiding this comment

alan-agius4 Mar 5, 2018

Choose a reason for hiding this comment

alexeagle Mar 5, 2018

Choose a reason for hiding this comment

alan-agius4 Mar 5, 2018 • edited

Choose a reason for hiding this comment

alan-agius4 Mar 6, 2018 • edited

Choose a reason for hiding this comment

alan-agius4 Mar 5, 2018

Choose a reason for hiding this comment

alexeagle Mar 9, 2018

Choose a reason for hiding this comment

alan-agius4 Mar 9, 2018

Choose a reason for hiding this comment

alexeagle commented Mar 7, 2018

alexeagle commented Mar 7, 2018

ngbot bot commented Mar 7, 2018

alan-agius4 commented Mar 7, 2018 via email • edited

alan-agius4 commented Mar 7, 2018

alexeagle left a comment

Choose a reason for hiding this comment

alexeagle Mar 9, 2018

Choose a reason for hiding this comment

IgorMinar left a comment

Choose a reason for hiding this comment

alan-agius4 commented Mar 9, 2018

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

alan-agius4 commented Mar 3, 2018 •

edited

alfaproject Mar 3, 2018 •

edited

alan-agius4 commented Mar 3, 2018 •

edited

alan-agius4 Mar 5, 2018 •

edited

alan-agius4 Mar 6, 2018 •

edited

alan-agius4 commented Mar 7, 2018 via email •

edited