Skip to content

Conversation

devversion
Copy link
Member

@devversion devversion commented Mar 26, 2021

Creates a CI job to ensure compatibility with the ng-linker.

@google-cla google-cla bot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Mar 26, 2021
@devversion devversion added the in progress This issue is currently in progress label Mar 26, 2021
@devversion devversion added the area: build & ci Related the build and CI infrastructure of the project label Mar 26, 2021
@devversion devversion force-pushed the build/add-angular-linker-compability-test branch from 94deda9 to 3c3b1b1 Compare April 7, 2021 19:13
@devversion
Copy link
Member Author

This will be unblocked with angular/angular#41583

@devversion devversion force-pushed the build/add-angular-linker-compability-test branch 3 times, most recently from 8b9186b to 51606a3 Compare April 21, 2021 20:37
@devversion devversion force-pushed the build/add-angular-linker-compability-test branch from 51606a3 to 0e47338 Compare April 27, 2021 17:19
@devversion devversion marked this pull request as ready for review April 27, 2021 20:13
@devversion devversion requested review from jelbourn and a team as code owners April 27, 2021 20:13
@devversion devversion added target: rc This PR is targeted for the next release-candidate and removed in progress This issue is currently in progress labels Apr 27, 2021
@devversion
Copy link
Member Author

This is now unblocked and ready for review. cc. @jelbourn @josephperrott

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

Though I am not the right person to confirm this is the correct/best way to test the ng-linker.

@devversion devversion force-pushed the build/add-angular-linker-compability-test branch from 0e47338 to ec3b651 Compare April 28, 2021 20:22
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.

Overall LGTM, you can add merge ready when ready

cc @petebacondarwin just FYI

Copy link

@petebacondarwin petebacondarwin left a comment

Choose a reason for hiding this comment

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

I have some views on some of the naming, which you might like to consider.

I note that this only tests that the partial declarations were processed without error, right? Are there plans to run some e2e tests, or similar, on the resulting linked code?

@devversion devversion force-pushed the build/add-angular-linker-compability-test branch 2 times, most recently from 87b2a7a to 313e905 Compare April 29, 2021 20:01
@devversion devversion force-pushed the build/add-angular-linker-compability-test branch 5 times, most recently from 5ff4a07 to dae982c Compare April 30, 2021 08:34
@devversion
Copy link
Member Author

devversion commented Apr 30, 2021

@petebacondarwin I initially considered running e2e tests with the linker, but thought it would be quite tricky/require a lot of refactoring to get that running with our Bazel setup.

I've spent more time on this, and got the e2e tests working with partial compilation enabled.

@petebacondarwin @jelbourn @josephperrott Please have a look at the second commit.

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

Copy link

@petebacondarwin petebacondarwin left a comment

Choose a reason for hiding this comment

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

LGTM2

load("@npm//@bazel/concatjs:index.bzl", "concatjs_devserver")
load("//:packages.bzl", "getAngularUmdTargets")
load("//tools:defaults.bzl", "ng_module", "sass_binary")

package(default_visibility = ["//visibility:public"])

# List of dependencies that are referenced in the `index.html` file.
devserverIndexHtmlDependencies = [

Choose a reason for hiding this comment

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

I read this as devversionIndexHtmlDependencies and thought, why...? 😆

* which are not statically analyzable. We work around this by transforming the
* dynamic non-static imports to statically analyzable dynamic imports that can be
* processed by rollup. e.g. "import(`@angular/components-examples/${module}/index.js`")
* will be transformed into an object that merges exports from all possible `${module}` values.

Choose a reason for hiding this comment

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

It might be worth noting for the future, that if you have @angular/compiler loaded at runtime, then partial declarations can be JIT compiled to full definitions, without the need for the linker...

Copy link
Member Author

Choose a reason for hiding this comment

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

I see. good to know! I'm not sure what's best here; but I imagine the JIT compilation slowing down tests a little bit more (if not significantly)

@devversion devversion added the action: merge The PR is ready for merge by the caretaker label Apr 30, 2021
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

@devversion devversion added the P2 The issue is important to a large percentage of users, with a workaround label May 3, 2021
@devversion devversion force-pushed the build/add-angular-linker-compability-test branch from 1b7db3f to 187d28a Compare May 3, 2021 18:26
@devversion devversion force-pushed the build/add-angular-linker-compability-test branch from 187d28a to 83f9628 Compare May 11, 2021 20:27
@mmalerba mmalerba added target: patch This PR is targeted for the next patch release and removed target: rc This PR is targeted for the next release-candidate labels May 13, 2021
@mmalerba
Copy link
Contributor

@devversion needs rebase

Adds tests to ensure that Angular components is compatible with
Angular's patial compilation mode and the linker.

A new job running every hour ensures that changes to the partial
compilation or linker in Angular `master` are compatible with the
components repository. Additionally, we add a job that runs for
each commit/PR in order to insure that Angular components remains
compatible with the linker of already-released Angular versions.
@devversion devversion force-pushed the build/add-angular-linker-compability-test branch from 83f9628 to b020b3e Compare May 13, 2021 17:56
@devversion
Copy link
Member Author

@mmalerba Done 👍

@andrewseguin andrewseguin merged commit 7d53a6d into angular:master May 17, 2021
andrewseguin pushed a commit that referenced this pull request May 17, 2021
* build: add tests to ensure compatibility with ng-linker

Adds tests to ensure that Angular components is compatible with
Angular's patial compilation mode and the linker.

A new job running every hour ensures that changes to the partial
compilation or linker in Angular `master` are compatible with the
components repository. Additionally, we add a job that runs for
each commit/PR in order to insure that Angular components remains
compatible with the linker of already-released Angular versions.

* build: build and run e2e tests in partial compilation mode

* fixup! build: build and run e2e tests in partial compilation mode

Address feedback

(cherry picked from commit 7d53a6d)
@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 Jun 17, 2021
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 area: build & ci Related the build and CI infrastructure of the project cla: yes PR author has agreed to Google's Contributor License Agreement P2 The issue is important to a large percentage of users, with a workaround target: patch This PR is targeted for the next patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants