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(ivy): inherit static coercion members from base classes #34296

Closed
wants to merge 2 commits into from

Conversation

@JoostK
Copy link
Member

JoostK commented Dec 7, 2019

See individual commits for descriptions

Fixes #33830

For Ivy's template type checker it is possible to let a directive
specify static members to allow a wider type for some input:

```typescript
export class MatSelect {
  @input() disabled: boolean;

  static ngAcceptInputType_disabled: boolean | string;
}
```

This allows a binding to the `MatSelect.disabled` input to be of type
boolean or string, whereas the `disabled` property itself is only of
type boolean.

Up until now, any static `ngAcceptInputType_*` property was not
inherited for subclasses of a directive class. This is cumbersome, as
the directive's inputs are inherited, so any acceptance member should as
well. To resolve this limitation, this commit extends the flattening of
directive metadata to include the acceptance members.

Fixes #33830
Resolves FW-1759
@ngbot ngbot bot added this to the needsTriage milestone Dec 7, 2019
@googlebot googlebot added the cla: yes label Dec 7, 2019
@JoostK JoostK changed the title Ngtsc ttc coercion fix(ivy): inherit static coercion members from base classes Dec 7, 2019
@JoostK JoostK force-pushed the JoostK:ngtsc-ttc-coercion branch from 70f2199 to d1a8060 Dec 8, 2019
The metadata collector for View Engine compilations emits error symbols
for static class members that have not been initialized, which prevents
a library from building successfully when `strictMetadataEmit` is
enabled, which is recommended for libraries to avoid issues in library
consumers. This is troublesome for libraries that are adopting static
members for the Ivy template type checker: these members don't need a
value assignment as only their type is of importance, however this
causes metadata errors. As such, a library used to be required to
initialize the special static members to workaround this error,
undesirably introducing a code-size overhead in terms of emitted
JavaScript code.

This commit modifies the collector logic to specifically ignore
the special static members for Ivy's template type checker, preventing
any errors from being recorded during the metadata collection.
@JoostK JoostK force-pushed the JoostK:ngtsc-ttc-coercion branch from d1a8060 to a78bc49 Dec 8, 2019
@JoostK JoostK marked this pull request as ready for review Dec 8, 2019
@JoostK JoostK requested a review from angular/fw-compiler as a code owner Dec 8, 2019
devversion added a commit to devversion/material2 that referenced this pull request Dec 10, 2019
Previously Angular did not inherit coercion acceptance members until we submitted an issue
on the framework side. This issue will be fixed with angular/angular#34296.

In preparation for that change to land, this PR removes all unnecessary acceptance members and
updates the lint rule to avoid future duplications (the rule is also used to make that refactoring)
@alxhub
alxhub approved these changes Dec 10, 2019
Copy link
Contributor

alxhub left a comment

Nice work, this will be super helpful.

@alxhub

This comment has been minimized.

Copy link
Contributor

alxhub commented Dec 10, 2019

@ngbot

This comment has been minimized.

Copy link

ngbot bot commented Dec 10, 2019

I see that you just added the PR action: merge label, but the following checks are still failing:
    failure status "google3" is failing

If you want your PR to be merged, it has to pass all the CI checks.

If you can't get the PR to a green state due to flakes or broken master, please try rebasing to master and/or restarting the CI job. If that fails and you believe that the issue is not due to your change, please contact the caretaker and ask for help.

AndrewKushnir added a commit that referenced this pull request Dec 11, 2019
For Ivy's template type checker it is possible to let a directive
specify static members to allow a wider type for some input:

```typescript
export class MatSelect {
  @input() disabled: boolean;

  static ngAcceptInputType_disabled: boolean | string;
}
```

This allows a binding to the `MatSelect.disabled` input to be of type
boolean or string, whereas the `disabled` property itself is only of
type boolean.

Up until now, any static `ngAcceptInputType_*` property was not
inherited for subclasses of a directive class. This is cumbersome, as
the directive's inputs are inherited, so any acceptance member should as
well. To resolve this limitation, this commit extends the flattening of
directive metadata to include the acceptance members.

Fixes #33830
Resolves FW-1759

PR Close #34296
AndrewKushnir added a commit that referenced this pull request Dec 11, 2019
…#34296)

The metadata collector for View Engine compilations emits error symbols
for static class members that have not been initialized, which prevents
a library from building successfully when `strictMetadataEmit` is
enabled, which is recommended for libraries to avoid issues in library
consumers. This is troublesome for libraries that are adopting static
members for the Ivy template type checker: these members don't need a
value assignment as only their type is of importance, however this
causes metadata errors. As such, a library used to be required to
initialize the special static members to workaround this error,
undesirably introducing a code-size overhead in terms of emitted
JavaScript code.

This commit modifies the collector logic to specifically ignore
the special static members for Ivy's template type checker, preventing
any errors from being recorded during the metadata collection.

PR Close #34296
AndrewKushnir added a commit that referenced this pull request Dec 11, 2019
…#34296)

The metadata collector for View Engine compilations emits error symbols
for static class members that have not been initialized, which prevents
a library from building successfully when `strictMetadataEmit` is
enabled, which is recommended for libraries to avoid issues in library
consumers. This is troublesome for libraries that are adopting static
members for the Ivy template type checker: these members don't need a
value assignment as only their type is of importance, however this
causes metadata errors. As such, a library used to be required to
initialize the special static members to workaround this error,
undesirably introducing a code-size overhead in terms of emitted
JavaScript code.

This commit modifies the collector logic to specifically ignore
the special static members for Ivy's template type checker, preventing
any errors from being recorded during the metadata collection.

PR Close #34296
devversion added a commit to devversion/material2 that referenced this pull request Dec 12, 2019
Previously Angular did not inherit coercion acceptance members until we submitted an issue
on the framework side. This issue will be fixed with angular/angular#34296.

In preparation for that change to land, this PR removes all unnecessary acceptance members and
updates the lint rule to avoid future duplications (the rule is also used to make that refactoring)
devversion added a commit to devversion/material2 that referenced this pull request Dec 31, 2019
Previously Angular did not inherit coercion acceptance members until
we submitted an issue on the framework side. This issue will be fixed with
angular/angular#34296.

In preparation for that change to land, this PR removes all unnecessary
acceptance members and updates the lint rule to avoid future duplications
(the rule is also used to make that refactoring)
devversion added a commit to devversion/material2 that referenced this pull request Dec 31, 2019
Previously Angular did not inherit coercion acceptance members until
we submitted an issue on the framework side. This issue will be fixed with
angular/angular#34296.

In preparation for that change to land, this PR removes all unnecessary
acceptance members and updates the lint rule to avoid future duplications
(the rule is also used to make that refactoring)
@angular-automatic-lock-bot

This comment has been minimized.

Copy link

angular-automatic-lock-bot bot commented Jan 11, 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 11, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.