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

feat(upgrade): allowing attribute usage #29892

Closed

Conversation

Projects
None yet
4 participants
@banjankri
Copy link

banjankri commented Apr 14, 2019

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

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

#16695

Issue Number: #16695

What is the new behavior?

Allowing both attribute and element for downgraded components.

Does this PR introduce a breaking change?

  • Yes
  • [ x] No

Other information

@banjankri banjankri requested a review from angular/fw-upgrade as a code owner Apr 14, 2019

@googlebot googlebot added the cla: yes label Apr 14, 2019

@gkalpak

This comment has been minimized.

Copy link
Member

gkalpak commented Apr 15, 2019

I'm afraid we can't accept this PR as is. A change like this needs to be thought out, tested and documented.
(Also, as it is, it is a (possibly unnecessary) breaking change.)

Some concerns:

  1. While there is no practical limitation as to whether the selector of a downgraded component targets a tag name or an attribute, there is the limitation that each element cannot belong to two downgraded components. This is guaranteed to happen due to the terminal: true in downgradeComponent(). This means that only the first matched downgraded component will be applied.

    While it is true that you can define multiple components with the same element selector (of which only the first one will be applied), it is much easier to unintentionally do this with different attribute selectors, leading to harder to detect and debug failures.

  2. By indiscreetly applying restrict: 'EA' to all downgraded components is a breaking change (since some attributes that happen to match a component selector will start matching downgraded components that they didn't match before).

One option to address the above concerns would be to make it explicit when a downgraded component is supposed to match on attributes instead of (or in addition to) tag names. For example, ngUpgrade could export a new helper, downgradeAttributeComponent() (or something), to target attributes (e.g. see #16695 (comment)). Or, downgradeComponent() could accept an extra config option to allow matching attributes (e.g. downgradeComponent({..., matchAttributes: true}) or downgradeComponent({..., restrict: 'EA'})).

This would avoid the breaking change (since all existing code would continue to work as it did before and users would have to opt-in to matching attributes) and it would make it explicit/obvious that a particular downgraded component in supposed to match attributes.

WDYT, @banjankri?

BTW, whatever direction we decide to go, we'll need documentation and tests 😉
(In the meantime, it is easy to implement this outside of ngUpgrade: #16695 (comment) (but this doesn't mean we should not built this functionality into ngUpgrade as well))

@banjankri

This comment has been minimized.

Copy link
Author

banjankri commented Apr 15, 2019

Thank you very much for your feedback! Definitely agree with you on that this PR as is would change existing behaviour.

I like the options you mentioned and personally would lean towards the latest as it is giving most flexibility (additional paremeter to amend the the restrict configuration option). Wdyt if I pursue this path?

@banjankri

This comment has been minimized.

Copy link
Author

banjankri commented Apr 15, 2019

In the meantime closing this PR - will re-open once I have something ready.

@banjankri banjankri closed this Apr 15, 2019

@gkalpak

This comment has been minimized.

Copy link
Member

gkalpak commented Apr 17, 2019

I am a little concerned of opening up restrict entirely. E.g. I am not sure it is a good idea to allow downgraded components to be match on class-names, let alone comments 😁

But I like being able to specify either E only (the default) or A only or E and A.
One option might be introducing restrict, but only allowing E and A (and either ignore others or throw. We could even give a narrower type to help prevent invalid values: restrict: 'E' | 'A' | 'EA' | 'AE'

WDYT, @banjankri?

@petebacondarwin

This comment has been minimized.

Copy link
Member

petebacondarwin commented Apr 17, 2019

Also we do not want to confuse people into thinking that they can downgrade @Directive directives. Only Angular components (i.e. @Component) can be downgraded as ngUpgrade requires that they own the whole element.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.