Skip to content

refactor(ivy): remove ngBaseDef - #33264

Closed
crisbeto wants to merge 1 commit into
angular:masterfrom
crisbeto:FW-1613/base-def-remove
Closed

crisbeto wants to merge 1 commit into
angular:masterfrom
crisbeto:FW-1613/base-def-remove

Conversation

@crisbeto

Copy link
Copy Markdown
Member

Removes ngBaseDef from the compiler and any runtime code that was still referring to it. In the cases where we'd previously generate a base def we now generate a definition for an abstract directive.

@crisbeto
crisbeto force-pushed the FW-1613/base-def-remove branch 2 times, most recently from aa0e499 to bf5c2b2 Compare October 19, 2019 11:39
@crisbeto crisbeto added comp: ivy action: review The PR is still awaiting reviews from at least one requested reviewer target: major This PR is targeted for the next major release labels Oct 19, 2019
@ngbot ngbot Bot added this to the needsTriage milestone Oct 19, 2019
@crisbeto
crisbeto marked this pull request as ready for review October 19, 2019 12:06
@crisbeto
crisbeto requested review from a team October 19, 2019 12:06
@crisbeto
crisbeto force-pushed the FW-1613/base-def-remove branch from bf5c2b2 to 7f5b5a6 Compare October 21, 2019 15:19
@AndrewKushnir

AndrewKushnir commented Oct 21, 2019 •

Copy link
Copy Markdown
Contributor

Initial set of presubmits:

@AndrewKushnir AndrewKushnir added the action: presubmit The PR is in need of a google3 presubmit label Oct 21, 2019
@AndrewKushnir

Copy link
Copy Markdown
Contributor

Quick update after running presubmit in g3: it looks like there are some regressions, but they seem to be caused by the fact that there is a problem with inheriting inputs from base classes (annotated with @Directive()), which is used in this PR as a replacement for ngBaseDef. The problem should be resolved by PR #33131, but it's hard to tell whether it's the only problem in g3 or not. My proposal is to land PR #33131 first, rebase this PR and run g3 presubmit again (if time permits). As an option, we can create a draft PR that would include the changes from this PR and PR #33131 to see if there are any regressions in g3. Thank you.

@AndrewKushnir AndrewKushnir removed the action: presubmit The PR is in need of a google3 presubmit label Oct 22, 2019
@crisbeto

Copy link
Copy Markdown
Member Author

Are those compile-time errors or runtime? If I remember correctly, we have a lot of inheritance tests for the runtime which are passing.

@AndrewKushnir

Copy link
Copy Markdown
Contributor

@crisbeto yes, the errors happen at runtime, so I guess I was incorrect assuming that #33131 will fix those. Let me collect more information and get back with specific use-cases that regressed. Thank you.

@crisbeto
crisbeto force-pushed the FW-1613/base-def-remove branch from 7f5b5a6 to ac4de4b Compare October 24, 2019 12:48
@crisbeto
crisbeto force-pushed the FW-1613/base-def-remove branch 5 times, most recently from b87ed22 to c2d7743 Compare October 24, 2019 21:20
@crisbeto
crisbeto force-pushed the FW-1613/base-def-remove branch from c2d7743 to 2743778 Compare October 24, 2019 22:12
@AndrewKushnir

AndrewKushnir commented Oct 25, 2019 •

Copy link
Copy Markdown
Contributor

FYI, VE and Ivy presubmits were successful (g3 regression was resolved) at the point when I started them (see my comment here). I believe CI was "green" at that time as well, so I'm wondering if we can roll back recent changes (based on CR) in this PR if they are not critical, so we can unblock this PR. The remaining comments/improvements (if they are not critical) can be performed as a followup, especially given that we became blocked on #32987.

@crisbeto
crisbeto force-pushed the FW-1613/base-def-remove branch 2 times, most recently from a69d119 to 077f442 Compare October 25, 2019 18:56
@crisbeto
crisbeto requested a review from IgorMinar as a code owner October 25, 2019 18:56
@crisbeto
crisbeto requested review from a team October 25, 2019 18:56
@googlebot

Copy link
Copy Markdown

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.

@crisbeto
crisbeto force-pushed the FW-1613/base-def-remove branch from 077f442 to 402033e Compare October 25, 2019 19:04
@alxhub alxhub added cla: yes and removed cla: no labels Oct 25, 2019
@googlebot

Copy link
Copy Markdown

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

@alxhub

alxhub commented Oct 25, 2019

Copy link
Copy Markdown
Member

Caretaker: these are being done preemptively. Once #32987 lands, this should be in a good state.

Presubmit
Ivy Presubmit

Removes `ngBaseDef` from the compiler and any runtime code that was still referring to it. In the cases where we'd previously generate a base def we now generate a definition for an abstract directive.
@crisbeto
crisbeto force-pushed the FW-1613/base-def-remove branch from 402033e to e957e63 Compare October 25, 2019 19:17
@googlebot

Copy link
Copy Markdown

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@crisbeto crisbeto removed action: review The PR is still awaiting reviews from at least one requested reviewer state: blocked labels Oct 25, 2019
@alxhub alxhub added action: merge The PR is ready for merge by the caretaker merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note action: review The PR is still awaiting reviews from at least one requested reviewer labels Oct 25, 2019
@AndrewKushnir AndrewKushnir removed action: review The PR is still awaiting reviews from at least one requested reviewer action: presubmit The PR is in need of a google3 presubmit labels Oct 25, 2019
@gkalpak gkalpak mentioned this pull request Oct 28, 2019
22 of 45 tasks
@angular-automatic-lock-bot

Copy link
Copy Markdown

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

Labels

action: merge The PR is ready for merge by the caretaker cla: yes merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note target: major This PR is targeted for the next major release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants