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

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

Closed
wants to merge 4 commits into from
Closed

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

wants to merge 4 commits into from

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 as a code owner 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 as a code owner 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 review from IgorMinar and a team as code owners 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 as code owners February 22, 2019 14:42
@googlebot
Copy link

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

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

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

@mary-poppins
Copy link

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

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

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

@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

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

@mary-poppins
Copy link

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

@mary-poppins
Copy link

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 added the action: presubmit The PR is in need of a google3 presubmit label May 1, 2019
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

@kara kara removed the action: presubmit The PR is in need of a google3 presubmit label May 1, 2019
@kara kara closed this in 066ec33 May 1, 2019
kara pushed a commit that referenced this pull request May 1, 2019
@gkalpak @kara
fix(docs-infra): update app code to work with Ivy (#28530)
b70d20b 
This commit also enables more tests to be run on CI with Ivy.

PR Close #28530
kara pushed a commit that referenced this pull request May 1, 2019
@gkalpak @kara
fix(ivy): allow R3TestBedCompiler to work in ngcc-processed apps (#…
7cbc69c 
…28530)

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 #30121 for details).

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

Fixes #30121

PR Close #28530
kara pushed a commit that referenced this pull request May 1, 2019
@gkalpak @kara
fix(docs-infra): update app code to work with Ivy (#28530)
28e4187 
This commit also enables more tests to be run on CI with Ivy.

PR Close #28530
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
@@ -23,10 +23,10 @@ describe('CodeTabsComponent', () => {
});

fixture = TestBed.createComponent(HostComponent);
fixture.detectChanges();

hostComponent = fixture.componentInstance;
Copy link
Contributor

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

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 gkalpak deleted the ci-aio-run-with-ivy branch May 3, 2019 03:33
@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 😁

BioPhoton pushed a commit to BioPhoton/angular that referenced this pull request May 21, 2019
@gkalpak @BioPhoton
fix(ivy): allow R3TestBedCompiler to work in ngcc-processed apps (a…
891d162 
…ngular#28530)

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

PR Close angular#28530
BioPhoton pushed a commit to BioPhoton/angular that referenced this pull request May 21, 2019
@gkalpak @BioPhoton
fix(docs-infra): update app code to work with Ivy (angular#28530)
f24b12e 
This commit also enables more tests to be run on CI with Ivy.

PR Close angular#28530
@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 15, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@IgorMinar IgorMinar IgorMinar left review comments

@filipesilva filipesilva filipesilva left review comments

@kara kara kara approved these changes

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