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.

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

fix(core): support Attribute DI decorator in deps section of a token #37085

Closed
wants to merge 4 commits into from
Closed

fix(core): support Attribute DI decorator in deps section of a token #37085

wants to merge 4 commits into from

Conversation

sonukapoor
Copy link
Contributor

@sonukapoor sonukapoor commented May 13, 2020 •
edited by JoostK

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • 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?

Issue Number: Fixes #36479

What is the new behavior?

Does this PR introduce a breaking change?

  • Yes
  • No

@sonukapoor sonukapoor force-pushed the treeshaked-attribute-injection branch 3 times, most recently from 7ae08ef to 03cfa98 Compare May 13, 2020 18:22
@mhevery mhevery added the area: core Issues related to the framework runtime label May 13, 2020
@ngbot ngbot bot added this to the needsTriage milestone May 13, 2020
@sonukapoor sonukapoor force-pushed the treeshaked-attribute-injection branch 2 times, most recently from 0489f41 to 014b123 Compare May 14, 2020 10:42
@AndrewKushnir AndrewKushnir changed the title fix(core): WIP - support Attribute DI decorator in deps section o… fix(core): support Attribute DI decorator in deps section of a token May 15, 2020
@AndrewKushnir AndrewKushnir mentioned this pull request May 15, 2020
Closed
14 tasks
@sonukapoor sonukapoor force-pushed the treeshaked-attribute-injection branch 5 times, most recently from 3d635a8 to ef89ae4 Compare September 9, 2020 11:56
@mary-poppins
Copy link

You can preview 7e37bd8 at https://pr37085-7e37bd8.ngbuilds.io/.
You can preview 279261b at https://pr37085-279261b.ngbuilds.io/.
You can preview 7ae08ef at https://pr37085-7ae08ef.ngbuilds.io/.
You can preview 03cfa98 at https://pr37085-03cfa98.ngbuilds.io/.
You can preview 0489f41 at https://pr37085-0489f41.ngbuilds.io/.
You can preview 014b123 at https://pr37085-014b123.ngbuilds.io/.
You can preview 85f400b at https://pr37085-85f400b.ngbuilds.io/.
You can preview 3d635a8 at https://pr37085-3d635a8.ngbuilds.io/.
You can preview ef89ae4 at https://pr37085-ef89ae4.ngbuilds.io/.
You can preview 3ccbb12 at https://pr37085-3ccbb12.ngbuilds.io/.

@mary-poppins
Copy link

You can preview 06303e9 at https://pr37085-06303e9.ngbuilds.io/.

@mary-poppins
Copy link

You can preview e67a164 at https://pr37085-e67a164.ngbuilds.io/.

@mary-poppins
Copy link

You can preview 0adbfe3 at https://pr37085-0adbfe3.ngbuilds.io/.

@sonukapoor sonukapoor marked this pull request as ready for review September 11, 2020 15:18
@pullapprove pullapprove bot requested a review from mhevery September 11, 2020 15:18
@AndrewKushnir
Copy link
Contributor

Initial presubmit.

@sonukapoor
Copy link
Contributor Author

@AndrewKushnir How did the presubmit go?

@sonukapoor @mhevery
refactor(core): move injectAttributeImpl to avoid cycles
6ac2d2a 
This commit moves the `injectAttributeImpl` and other dependent code
to avoid circular dependencies.
@sonukapoor
Copy link
Contributor Author

I have fixed up this PR for the circular deps issue.

Sweet. Thanks @mhevery

@mhevery mhevery force-pushed the treeshaked-attribute-injection branch from dfc8c2c to 6ac2d2a Compare November 18, 2020 20:59
mhevery
mhevery approved these changes Nov 18, 2020
View reviewed changes
Copy link
Contributor

@mhevery mhevery left a comment

Choose a reason for hiding this comment

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

reviewed-for: global-approvers

@mhevery mhevery added the action: merge The PR is ready for merge by the caretaker label Nov 18, 2020
@mhevery
Copy link
Contributor

mhevery commented Nov 18, 2020

presubmit

josephperrott
josephperrott approved these changes Nov 18, 2020
View reviewed changes
Copy link
Member

@josephperrott josephperrott left a comment

Choose a reason for hiding this comment

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

LGTM

@AndrewKushnir AndrewKushnir 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 Nov 18, 2020
@AndrewKushnir
Copy link
Contributor

FYI, I'm adding the "cleanup" labels since the CI is failing and the g3 check is not "green". Thank you.

@mhevery
Copy link
Contributor

mhevery commented Nov 19, 2020

presubmit deflake

@google-cla
Copy link

google-cla bot commented Nov 19, 2020

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@google-cla google-cla bot added cla: no and removed cla: yes labels Nov 19, 2020
@mhevery mhevery added cla: yes and removed cla: no action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Nov 19, 2020
AndrewKushnir pushed a commit that referenced this pull request Nov 19, 2020
@sonukapoor @AndrewKushnir
refactor(core): move injectAttributeImpl to avoid cycles (#37085)
be998e8 
This commit moves the `injectAttributeImpl` and other dependent code
to avoid circular dependencies.

PR Close #37085
AndrewKushnir pushed a commit that referenced this pull request Nov 19, 2020
@sonukapoor @AndrewKushnir
fix(core): support Attribute DI decorator in deps section of a to…
aaa3111 
…ken (#37085)

This commit fixes a bug when `Attribute` DI decorator is used in the
`deps` section of a token that uses a factory function. The problem
appeared because the `Attribute` DI decorator was not handled correctly
while injecting factory function attributes.

Closes #36479

PR Close #37085
AndrewKushnir pushed a commit that referenced this pull request Nov 19, 2020
@sonukapoor @AndrewKushnir
refactor(core): move injectAttributeImpl to avoid cycles (#37085)
2506c02 
This commit moves the `injectAttributeImpl` and other dependent code
to avoid circular dependencies.

PR Close #37085
@sonukapoor sonukapoor deleted the treeshaked-attribute-injection branch November 19, 2020 20:28
@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 Dec 20, 2020
@pullapprove pullapprove bot removed the area: core Issues related to the framework runtime label Dec 20, 2020
@ngbot ngbot bot removed this from the needsTriage milestone Dec 20, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@mhevery mhevery mhevery approved these changes

@AndrewKushnir AndrewKushnir AndrewKushnir left review comments

@josephperrott josephperrott josephperrott approved these changes

Assignees

@mhevery mhevery

@AndrewKushnir AndrewKushnir

Labels
action: merge The PR is ready for merge by the caretaker cla: yes core: di target: patch This PR is targeted for the next patch release
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Attribute in deps stopped working with Ivy
8 participants
@sonukapoor @mary-poppins @AndrewKushnir @mhevery @josephperrott @petebacondarwin @googlebot @gkalpak