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

refactor(ivy): remove ngBaseDef #33264

Closed
wants to merge 1 commit into from

Conversation

crisbeto
Copy link
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 as code owners October 19, 2019 12:06
@AndrewKushnir
Copy link
Contributor

AndrewKushnir commented Oct 21, 2019

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
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
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
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.

@AndrewKushnir
Copy link
Contributor

AndrewKushnir commented Oct 25, 2019

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 review from IgorMinar and a team as code owners October 25, 2019 18:56
@googlebot
Copy link

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.

@googlebot
Copy link

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

alxhub commented Oct 25, 2019

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.
@googlebot
Copy link

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
@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 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.

None yet

6 participants