Skip to content

Conversation

devversion
Copy link
Member

We currently only run tests against Ivy due to the
commit that switched the --define=compile flag.

This means that the ivy_test and local_browsers job
are doing the same. Instead we should change ivy_test to
run tests against View Engine. Same applies for the cronjobs.

@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Oct 24, 2019
@devversion devversion force-pushed the build/cleanup-ivy-ci-tests branch 3 times, most recently from 2211afa to 8cb2490 Compare October 24, 2019 12:43
We currently only run tests against Ivy due to the
commit that switched the `--define=compile` flag.

This means that the `ivy_test` and `local_browsers` job
are doing the same. Instead we should change `ivy_test` to
run tests against View Engine. Same applies for the cronjobs.
@devversion devversion force-pushed the build/cleanup-ivy-ci-tests branch from 8cb2490 to dda39e4 Compare October 24, 2019 13:30
@devversion
Copy link
Member Author

devversion commented Oct 24, 2019

With this PR, it's also possible to switch between Ivy and View Engine easily by just passing --define=compile=legacy. I'm not 100% happy about the postinstall patch, but our setup is special since we do all of this in Bazel, and by default the pre-render NodeJS target should run with Ivy.. there is no easy way to hack/overwrite the module resolution like we can in browsers.

Since it's temporary and reasonable that we default to Ivy bundles, I think it's fine like that.

@devversion devversion marked this pull request as ready for review October 24, 2019 13:58
@devversion devversion requested review from a team and jelbourn as code owners October 24, 2019 13:58
@devversion devversion added pr: merge safe target: major This PR is targeted for the next major release labels Oct 24, 2019
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

@josephperrott josephperrott added pr: lgtm action: merge The PR is ready for merge by the caretaker labels Oct 24, 2019
Copy link
Member

@jelbourn jelbourn left a comment

Choose a reason for hiding this comment

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

LGTM

@jelbourn jelbourn merged commit 71b99a0 into angular:master Oct 24, 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 24, 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 PR author has agreed to Google's Contributor License Agreement target: major This PR is targeted for the next major release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants