Skip to content

fix(common): add @Injectable() to common pipes #29834

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

fix(common): add @Injectable() to common pipes #29834

wants to merge 1 commit into from
+22 −9

Conversation

ocombe
Copy link
Contributor

@ocombe ocombe commented Apr 11, 2019 •
edited
Loading

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

Sorry, something went wrong.

@ocombe ocombe added feature Issue that requests a new feature area: common Issues related to APIs in the @angular/common package target: major This PR is targeted for the next major release labels Apr 11, 2019
@ocombe ocombe requested review from a team as code owners April 11, 2019 13:52
@ngbot ngbot bot added this to the needsTriage milestone Apr 11, 2019
pkozlowski-opensource
pkozlowski-opensource approved these changes Apr 11, 2019
View reviewed changes
Copy link
Member

@pkozlowski-opensource pkozlowski-opensource left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

kara
kara approved these changes Apr 11, 2019
View reviewed changes
Copy link
Contributor

@kara kara left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@kara
Copy link
Contributor

kara commented Apr 11, 2019 •
edited
Loading

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

@kara kara added the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Apr 11, 2019
@ocombe
fix(common): add @Injectable() to common pipes

Verified

This commit was signed with the committer’s verified signature.
imtayadeway Tim Wade
GPG key ID: D734FF7619F4129A
Verified
Learn about vigilant mode
8a109f4
@ocombe ocombe added type: bug/fix and removed feature Issue that requests a new feature action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Apr 11, 2019
@ocombe ocombe force-pushed the feat/fw-1242-injectable-pipes branch from 7242193 to 8a109f4 Compare April 11, 2019 14:12
@kara kara changed the title feat(common): add @Injectable() to common pipes fix(common): add @Injectable() to common pipes Apr 11, 2019
@kara kara added action: merge The PR is ready for merge by the caretaker action: presubmit The PR is in need of a google3 presubmit labels Apr 11, 2019
@kara
Copy link
Contributor

kara commented Apr 11, 2019

presubmit

@IgorMinar IgorMinar removed the request for review from AndrewKushnir April 11, 2019 15:26
@IgorMinar IgorMinar closed this in 387fbb8 Apr 11, 2019
@ocombe ocombe deleted the feat/fw-1242-injectable-pipes branch April 11, 2019 16:00
@Airblader
Copy link
Contributor

Airblader commented Apr 13, 2019 •
edited
Loading

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
Copy link
Contributor Author

ocombe commented Apr 13, 2019 •
edited
Loading

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
Copy link
Contributor

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 pushed a commit to wKoza/angular that referenced this pull request Apr 17, 2019
@ocombe @wKoza
fix(common): add @Injectable() to common pipes (angular#29834)

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
GPG key ID: 4AEE18F83AFDEB23
Expired
Verified
Learn about vigilant mode
83095d6 
PR Close angular#29834
@angular-automatic-lock-bot
Copy link

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 Sep 14, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@pkozlowski-opensource pkozlowski-opensource

@kara kara

Assignees
No one assigned
Labels
action: merge The PR is ready for merge by the caretaker action: presubmit The PR is in need of a google3 presubmit area: common Issues related to APIs in the @angular/common package cla: yes target: major This PR is targeted for the next major release type: bug/fix
Projects
None yet
Milestone
needsTriage
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@ocombe @kara @Airblader @pkozlowski-opensource @googlebot