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(core): missing-injectable migration should migrate empty object literal providers #33709

Conversation

@devversion
Copy link
Member

devversion commented Nov 9, 2019

In View Engine, providers which neither used useValue, useClass,
useFactory or useExisting, were interpreted differently.

e.g.

{provide: X} -> {provide: X, useValue: undefined}, // this is how it works in View Engine
{provide: X} -> {provide: X, useClass: X}, // this is how it works in Ivy

The missing-injectable migration should migrate such providers to the
explicit useValue provider. This ensures that there is no unexpected
behavioral change when updating to v9.

See: https://next.angular.io/guide/ivy-compatibility#less-common-changes

@googlebot googlebot added the cla: yes label Nov 9, 2019
@devversion devversion force-pushed the devversion:feat/missing-injectable-migrate-empty-object-literal-providers branch 4 times, most recently from c27f980 to 795aaae Nov 9, 2019
@devversion devversion marked this pull request as ready for review Nov 12, 2019
@devversion devversion requested review from angular/fw-compiler as code owners Nov 12, 2019
@ngbot ngbot bot modified the milestone: needsTriage Nov 12, 2019
@devversion

This comment has been minimized.

Copy link
Member Author

devversion commented Nov 12, 2019

The schematic ng-update migration (this PR) should be actually good to go. The question whether we should add such a migration to ngcc is still open. I'd need some feedback on that.

This PR also makes the visitObjectLiteralExpression method of StaticIntepreter protected, so that it can be overwritten by derived classes. Not sure if that is desired, but I think the interpreter is very powerful and it would be useful to do that for all methods. Happy to send a PR. cc. @alxhub

Also it currently won't be possible to do something in ngcc migrations without refactoring the ngcc migration infrastructure. Though, here is a proof that we could get it working: See here

@devversion devversion modified the milestones: needsTriage, v9-candidates Nov 13, 2019
@kara
kara approved these changes Nov 13, 2019
Copy link
Contributor

kara left a comment

LGTM, once typo is fixed

@kara kara requested a review from alxhub Nov 13, 2019
…literal providers

In View Engine, providers which neither used `useValue`, `useClass`,
`useFactory` or `useExisting`, were interpreted differently.

e.g.

```
{provide: X} -> {provide: X, useValue: undefined}, // this is how it works in View Engine
{provide: X} -> {provide: X, useClass: X}, // this is how it works in Ivy
```

The missing-injectable migration should migrate such providers to the
explicit `useValue` provider. This ensures that there is no unexpected
behavioral change when updating to v9.
@devversion devversion force-pushed the devversion:feat/missing-injectable-migrate-empty-object-literal-providers branch from 795aaae to eaa9cb3 Nov 14, 2019
@IgorMinar

This comment has been minimized.

Copy link
Member

IgorMinar commented Nov 16, 2019

do we need to add this to the list of migrations in https://next.angular.io/guide/updating-to-version-9#automated-migrations-for-version-9 ?

@alxhub
alxhub approved these changes Nov 16, 2019
Copy link
Contributor

alxhub left a comment

👍

I don't think we need to address this on the ngcc side. Every example of this kind of provider that I've seen has been from people trying to mock things out in tests. I doubt there are many libraries out there with unexpected undefined providers from this, as that would cause runtime failures.

@devversion

This comment has been minimized.

Copy link
Member Author

devversion commented Nov 16, 2019

@alxhub Sounds reasonable to me. Thanks for the feedback!

@IgorMinar Yes. We should add this to the list of migrations. When I chatted with @kara about handling this pattern in an migration, we talked about putting it into the existing missing-injectable migration since we do not want to create a separate migration w/ separate migration guide.

I think we should add it to the missing-injectable migration guide, but that somehow feels confusing since the pattern this PR handles is not necessarily related to a missing @Injectable() (I think we still need to decide about this, so I guess we can do that in a follow-up?)

alxhub added a commit that referenced this pull request Nov 18, 2019
…literal providers (#33709)

In View Engine, providers which neither used `useValue`, `useClass`,
`useFactory` or `useExisting`, were interpreted differently.

e.g.

```
{provide: X} -> {provide: X, useValue: undefined}, // this is how it works in View Engine
{provide: X} -> {provide: X, useClass: X}, // this is how it works in Ivy
```

The missing-injectable migration should migrate such providers to the
explicit `useValue` provider. This ensures that there is no unexpected
behavioral change when updating to v9.

PR Close #33709
@alxhub alxhub closed this in 15fefdb Nov 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.