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(common): add `@Injectable()` to common pipes #29834

Closed
wants to merge 1 commit into from

Conversation

@ocombe
Copy link
Contributor

commented Apr 11, 2019

PR Type

What kind of change does this PR introduce?

  • Feature

What is the current behavior?

Common pipes don't have the @Injectable decorator.

Issue Number: FW-1242

What is the new behavior?

Common pipes have the @Injectable decorator which makes them injectable for ivy.

Does this PR introduce a breaking change?

  • No

@ocombe ocombe requested review from angular/fw-core as code owners Apr 11, 2019

@ngbot ngbot bot added this to the needsTriage milestone Apr 11, 2019

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

@pkozlowski-opensource
Copy link
Member

left a comment

LGTM (provided that we actually want to inject all the pipes...)

@kara

kara approved these changes Apr 11, 2019

Copy link
Contributor

left a comment

LGTM

@kara

This comment has been minimized.

Copy link
Contributor

commented Apr 11, 2019

@ocombe Can you change the commit message to fix(common)? This is already possible in View Engine.

@ocombe ocombe force-pushed the ocombe:feat/fw-1242-injectable-pipes branch from 7242193 to 8a109f4 Apr 11, 2019

@kara kara changed the title feat(common): add `@Injectable()` to common pipes fix(common): add `@Injectable()` to common pipes Apr 11, 2019

@kara

This comment has been minimized.

Copy link
Contributor

commented Apr 11, 2019

@IgorMinar IgorMinar removed the request for review from AndrewKushnir Apr 11, 2019

@IgorMinar IgorMinar closed this in 387fbb8 Apr 11, 2019

@ocombe ocombe deleted the ocombe:feat/fw-1242-injectable-pipes branch Apr 11, 2019

@Airblader

This comment has been minimized.

Copy link
Contributor

commented Apr 13, 2019

Sorry to interfere here, but if this is required with Ivy, isn't that a breaking change (in Ivy) to the current engine where these decorators where explicitly not required (having @Pipe was enough)? If so, I thought Ivy was supposed to be a drop-in replacement, and also does this also mean we cannot inject components anymore unless they are explicitly decorated to be injectable?

@ocombe

This comment has been minimized.

Copy link
Contributor Author

commented Apr 13, 2019

I think that it's only an issue if you use renderComponent which is a different new kind of way to bootstrap your application (and thus not a breaking change).
But I'm not 100% sure, @kara can you confirm (or explain it better) please?

@Airblader

This comment has been minimized.

Copy link
Contributor

commented Apr 13, 2019

OK, that would make sense. To elaborate, I got here from reading the CHANGELOG which reads a bit unfortunate if this is the case. :-)

wKoza added a commit to wKoza/angular that referenced this pull request Apr 17, 2019

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.