Skip to content

ci(docs-infra): run with Ivy#28530

Closed
gkalpak wants to merge 4 commits intoangular:masterfrom
gkalpak:ci-aio-run-with-ivy
Closed

ci(docs-infra): run with Ivy#28530
gkalpak wants to merge 4 commits intoangular:masterfrom
gkalpak:ci-aio-run-with-ivy

Conversation

@gkalpak
Copy link
Member

@gkalpak gkalpak commented Feb 4, 2019 •
edited
Loading

PR Checklist

PR Type

  • angular.io application / infrastructure changes

What is the new behavior?

Update aio to run with Ivy and enable more aio tests to be run on CI with Ivy.

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

Jira issue: AVM-15

Blocked on:

@gkalpak gkalpak requested a review from a team February 4, 2019 22:28
@ngbot ngbot bot added this to the needsTriage milestone Feb 8, 2019
@gkalpak gkalpak force-pushed the ci-aio-run-with-ivy branch from 70de12b to 1bcc381 Compare February 12, 2019 07:47
@gkalpak gkalpak requested a review from a team February 12, 2019 07:47
@gkalpak gkalpak force-pushed the ci-aio-run-with-ivy branch 2 times, most recently from a452d5b to 00d90c3 Compare February 15, 2019 20:57
@gkalpak gkalpak requested a review from IgorMinar as a code owner February 15, 2019 20:57
@gkalpak gkalpak requested review from a team February 15, 2019 20:57
@gkalpak gkalpak force-pushed the ci-aio-run-with-ivy branch from 00d90c3 to f6fd4ff Compare February 22, 2019 14:42
@gkalpak gkalpak requested review from a team February 22, 2019 14:42
@googlebot
Copy link

googlebot commented Feb 22, 2019

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that 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 here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

ℹ️ Googlers: Go here for more info.

@gkalpak gkalpak force-pushed the ci-aio-run-with-ivy branch from f6fd4ff to 17037e3 Compare March 14, 2019 12:21
@googlebot
Copy link

googlebot commented Mar 14, 2019

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@gkalpak gkalpak force-pushed the ci-aio-run-with-ivy branch from 07ba28b to d937ad4 Compare March 14, 2019 17:56
@mary-poppins
Copy link

mary-poppins commented Mar 14, 2019

You can preview 70de12b at https://pr28530-70de12b.ngbuilds.io/.
You can preview 17037e3 at https://pr28530-17037e3.ngbuilds.io/.

@mary-poppins
Copy link

mary-poppins commented Mar 14, 2019

You can preview d937ad4 at https://pr28530-d937ad4.ngbuilds.io/.

@gkalpak gkalpak force-pushed the ci-aio-run-with-ivy branch 2 times, most recently from faddee0 to a0829ad Compare March 16, 2019 15:19
@mary-poppins
Copy link

mary-poppins commented Mar 16, 2019

You can preview a0829ad at https://pr28530-a0829ad.ngbuilds.io/.

@gkalpak gkalpak force-pushed the ci-aio-run-with-ivy branch from a0829ad to e913f1d Compare March 16, 2019 15:34
@mary-poppins
Copy link

mary-poppins commented Mar 16, 2019

You can preview e913f1d at https://pr28530-e913f1d.ngbuilds.io/.

@gkalpak gkalpak force-pushed the ci-aio-run-with-ivy branch from e953b9b to ef63e75 Compare April 30, 2019 23:29
@mary-poppins
Copy link

mary-poppins commented Apr 30, 2019

You can preview ef63e75 at https://pr28530-ef63e75.ngbuilds.io/.

@mary-poppins
Copy link

mary-poppins commented May 1, 2019

You can preview 3105d83 at https://pr28530-3105d83.ngbuilds.io/.

@gkalpak gkalpak force-pushed the ci-aio-run-with-ivy branch from 3105d83 to 715db19 Compare May 1, 2019 16:25
@mary-poppins
Copy link

mary-poppins commented May 1, 2019

You can preview 715db19 at https://pr28530-715db19.ngbuilds.io/.

@gkalpak gkalpak force-pushed the ci-aio-run-with-ivy branch from 715db19 to aa760f3 Compare May 1, 2019 16:47
@mary-poppins
Copy link

mary-poppins commented May 1, 2019

You can preview aa760f3 at https://pr28530-aa760f3.ngbuilds.io/.

@gkalpak gkalpak force-pushed the ci-aio-run-with-ivy branch from aa760f3 to 1f9b0f0 Compare May 1, 2019 17:08
@mary-poppins
Copy link

mary-poppins commented May 1, 2019

You can preview 1f9b0f0 at https://pr28530-1f9b0f0.ngbuilds.io/.

gkalpak added 2 commits May 1, 2019 11:32
@gkalpak
fix(ivy): allow R3TestBedCompiler to work in ngcc-processed apps
add0d00 
Previously, `R3TestBedCompiler` was dynamically defining an
`@NgModule`-decorated `CompilerModule` class inside a method call.
Since ngcc only processes top-level classes, this class was not
transformed causing failures in unit tests (see angular#30121 for details).

This commit fixes it by using `compileNgModuleDefs()` directly (similar
to the fix in angular#30037).

Fixes angular#30121
@gkalpak
fix(docs-infra): update app code to work with Ivy
f7a127c 
This commit also enables more tests to be run on CI with Ivy.
@gkalpak gkalpak force-pushed the ci-aio-run-with-ivy branch from 1f9b0f0 to f7a127c Compare May 1, 2019 17:32
@mary-poppins
Copy link

mary-poppins commented May 1, 2019

You can preview f7a127c at https://pr28530-f7a127c.ngbuilds.io/.

@mary-poppins
Copy link

mary-poppins commented May 1, 2019

You can preview 187b7b4 at https://pr28530-187b7b4.ngbuilds.io/.

@mary-poppins
Copy link

mary-poppins commented May 1, 2019

You can preview 2fe38cc at https://pr28530-2fe38cc.ngbuilds.io/.

@gkalpak gkalpak added area: build & ci Related the build and CI infrastructure of the project effort1: hours 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 target: patch This PR is targeted for the next patch release risk: low and removed state: WIP labels May 1, 2019
@gkalpak
Copy link
Member Author

gkalpak commented May 1, 2019

merge-assistance: Requires g3sync presubmit.
merge-assistance: This PR doesn't require all the requested reviews any more (many things have been split out to separate PRs).

kara
kara approved these changes May 1, 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 (global)

@kara
Copy link
Contributor

kara commented May 1, 2019

presubmit

IgorMinar
IgorMinar reviewed May 2, 2019
View reviewed changes
Copy link
Contributor

@IgorMinar IgorMinar left a comment

Choose a reason for hiding this comment

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

Doesn't updating the app code imply a breaking change?

aio/src/app/custom-elements/code/code-tabs.component.spec.ts
fixture = TestBed.createComponent(HostComponent);
fixture.detectChanges();

hostComponent = fixture.componentInstance;
Copy link
Contributor

@IgorMinar IgorMinar May 2, 2019

Choose a reason for hiding this comment

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

It's this a breaking change?

Copy link
Member Author

@gkalpak gkalpak May 7, 2019

Choose a reason for hiding this comment

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

The componentInstance is still available before detectChanges(). It is the codeTabsComponent (which is a ViewChild query) that isn't.

This is related to the (known breaking) changes in ViewChild queries. We could have made the codeTabsComponent ViewChild static instead (and leave the test as is).

@gkalpak
Copy link
Member Author

gkalpak commented May 7, 2019

Doesn't updating the app code imply a breaking change?

I assume you are referring to the changes in SearchBoxComponent. These are related to the (known breaking) changes in ViewChild queries.

I had originally left the searchBox query as non-static in this PR and moved the logic from ngOnInit() to ngAfterViewInit(). The query was changed to static in another PR, so the change from ngOnInit() to ngAfterViewInit() was not any more necessary (but I missed it when rebasing the PR).

Anyway, I am going to leave it as is, since it doesn't make any difference (and I generally prefer accessing view query results in ngAfterViewInit()), but the good news is that there is no breaking change we didn't already know 😁

@angular-automatic-lock-bot
Copy link

angular-automatic-lock-bot bot commented Sep 15, 2019

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.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

3 more reviewers

@IgorMinar IgorMinar IgorMinar left review comments

@filipesilva filipesilva filipesilva left review comments

@kara kara kara approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

action: merge The PR is ready for merge by the caretaker area: build & ci Related the build and CI infrastructure of the project cla: yes effort1: hours merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note risk: low target: patch This PR is targeted for the next patch release

Projects

None yet

Milestone

needsTriage

Development

Successfully merging this pull request may close these issues.

7 participants

@gkalpak @googlebot @mary-poppins @kara @IgorMinar @filipesilva @mhevery