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

build: update to rules_nodejs v4.0.0-beta.0 #42760

Closed

Conversation

Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
@devversion
Copy link
Member

@devversion devversion commented Jul 3, 2021

Updates the Bazel NodeJS rules to v4.0.0-beta.0. This is necessary
so that the Angular components repo can update, and it's generally
good to stay as up-to-date as possible with the Bazel rules as it's
easy to fall behind, and updating early allows us to discover issues
affecting our tooling earlier (where they are easier to address due to
e.g. potential breaking change policy).

@ngbot ngbot bot added this to the Backlog milestone Jul 3, 2021
@google-cla google-cla bot added the cla: yes label Jul 3, 2021
@devversion devversion force-pushed the build/update-rules-nodejs-v4-beta branch 7 times, most recently from 20698d2 to db04451 Jul 6, 2021
@devversion devversion changed the title [WIP] build: update to rules_nodejs v4.0.0-beta.0 build: update to rules_nodejs v4.0.0-beta.0 Jul 6, 2021
@devversion devversion force-pushed the build/update-rules-nodejs-v4-beta branch 4 times, most recently from f36c349 to ddb9c93 Jul 7, 2021
@devversion devversion requested a review from josephperrott Jul 7, 2021
@devversion devversion marked this pull request as ready for review Jul 7, 2021
Copy link
Member

@josephperrott josephperrott left a comment

LGTM

Loading

gkalpak
gkalpak approved these changes Jul 8, 2021
@@ -94,14 +94,14 @@
"@types/yaml": "^1.9.7",
"@types/yargs": "^16.0.1",
"@webcomponents/custom-elements": "^1.1.0",
"angular": "npm:angular@1.8",
Copy link
Member

@gkalpak gkalpak Jul 8, 2021

Choose a reason for hiding this comment

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

Can you elaborate on why this change is needed? (I read the commit message, but didn't get it (probably due to my lack of bazel knowledge) 😅)

Loading

Copy link
Member Author

@devversion devversion Jul 8, 2021

Choose a reason for hiding this comment

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

View Engine generates factory files like the followed in Bazel (here an app module ngfactory file):

import * as i0 from "@angular/core";
import * as i1 from "./app.module";
import * as i2 from "./app.component";
import * as i3 from "angular/modules/benchmarks/src/class_bindings/app.component.ngfactory";

If you look at the i3 and i0 imports, you will see that those are not relative and need to be resolved by Rollup either using some custom resolution, or from the node_modules/. The Bazel NodeJS rules uses the node_modules/ folder here and create two symlinks links in the action node_modules e.g.

node_modules/
  @angular/core | <..>/execroot/angular/bazel-out/packages/core
  angular | <..>/execroot/angular

The symlink for node_modules/angular is currently not possible because the angular folder already exists for AngularJS, so in order to be able to have the Bazel workspace symlinked, we need to alias the angular dependency.

Loading

Copy link
Member

@gkalpak gkalpak Jul 10, 2021

Choose a reason for hiding this comment

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

But how is this different from other dependencies? Why does only angular have this issue?

Loading

Copy link
Member

@gkalpak gkalpak Jul 10, 2021

Choose a reason for hiding this comment

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

But how is this different from other dependencies? Why does only angular have this issue?

Loading

Copy link
Member Author

@devversion devversion Jul 10, 2021

Choose a reason for hiding this comment

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

Other dependencies are not symlinked, they just exist in the node_modules already. It's a special case for the angular package because the Bazel workspace is named similarly, and imports generated by Bazel rules use so-called manifest paths that start with the workspace name. These imports would incorrectly resolve to AngularJS while they should resolve to the workspace bazel-out instead. Hence the Bazel NodeJS rules try to symlink the workspace in the node modules (which highlights the issue where the symlink cannot be created because node_modules/angular is already acquired for AngularJS)

I'm happy to jump on a VC to elaborate more on this (and demonstrate the scenario), or we can continue chatting about it.

Loading

Copy link
Member

@gkalpak gkalpak Jul 10, 2021

Choose a reason for hiding this comment

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

Oh, I think I get it now. It is because the name of the angular NPM package happens to coincide with the name of the workspace, right. (So, it is just an issue with this workspace, but would not be an issue with a differently named workspace, right?)

Thx for bearing with me 😇

Loading

packages/bazel/src/ng_module.bzl Outdated Show resolved Hide resolved
Loading
@devversion devversion force-pushed the build/update-rules-nodejs-v4-beta branch from ddb9c93 to 0268ab9 Jul 8, 2021
@devversion
Copy link
Member Author

@devversion devversion commented Jul 8, 2021

Disabling pull approve since all the remaining reviews are needed for basic BUILD.bazel changes that could be considered changed as part of the dev-infra codeowner group.

Loading

@devversion devversion removed the request for review from AndrewKushnir Jul 8, 2021
@AndrewKushnir
Copy link
Contributor

@AndrewKushnir AndrewKushnir commented Jul 8, 2021

FYI adding the "presubmit" label based on the google3 status which indicates that @josephperrott started a presubmit. Please remove the "presubmit" label once presubmit is completed.

Loading

@devversion devversion force-pushed the build/update-rules-nodejs-v4-beta branch 2 times, most recently from 2ef0fa8 to 64119ac Jul 8, 2021
@devversion
Copy link
Member Author

@devversion devversion commented Jul 8, 2021

Thanks @AndrewKushnir, sounds good. Note that I updated commit changing @angular/bazel from feat to refactor so that we can land it in the patch branch (until our tooling has been updated to allow Bazel features to land in patch).

Loading

@AndrewKushnir
Copy link
Contributor

@AndrewKushnir AndrewKushnir commented Jul 9, 2021

@devversion it looks like there are conflicts with the main branch now (after merging other changes). Could you please rebase and resolve conflicts when you get a chance? Thank you.

Loading

@devversion devversion force-pushed the build/update-rules-nodejs-v4-beta branch from 64119ac to 73d29ac Jul 9, 2021
@devversion
Copy link
Member Author

@devversion devversion commented Jul 9, 2021

@AndrewKushnir Done!

Loading

devversion added 5 commits Jul 9, 2021
… be resolved

Rollup just prints a warning if an import cannot be resolved and ends up
being treated as an external dependency. This in combination with the
`silent = True` attribute for `rollup_bundle` means that bundles might
end up being extremely small without people noticing that it misses
actual imports.

To improve this situation, the warning is replaced by an error if
an import cannot be resolved.

This unveiles an issue with the `ng_rollup_bundle` macro from
dev-infra where imports in View Engine were not resolved but ended
up being treated as external. This did not prevent benchmarks using
this macro from working because the ConcatJS devserver had builtin
resolution for workspace manifest paths. Though given the new check
for no unresolved imports, this will now cause errors within Rollup, and
we need to fix the resolution. We can fix the issue by temporarily
enabling workspace linking. This does not have any performance
downsides.

To enable workspace linking (which we might need more often in the
future given the linker taking over patched module resolution), we
had to rename the `angular` dependency to a more specific one so
that the Angular linker could link into `node_modules/angular`.
We removed `bazel-toolchains` from the repository since dev-infra
provides the RBE platforms now (in a way where they are not reliant
on the Bazel version), so the comment in `.bazelversion` can be
removed.
Skydoc is no longer used as `@angular/bazel` is no longer a
public API. The Sass rules were only used in a single place
in the repo where Sass is not really needed and has just been
added by accident most likely. We want to remove the Sass dependency
in preparation for Rules NodeJS v4.x where the Sass rules currently
still use an older version of `@bazel/worker` that is incompatible.
This commit applies changes to `@angular/bazel` which are necessary
to support the Bazel NodeJS rules v4.0.0. The Bazel NodeJS rules
no longer support the `_tslibrary` option for the `LinkablePackageInfo`
provider and therefore we need to stop using it. Due to this removal,
we also need to add two new attributes called `package_name` and
`package_path` so that the API of `ng_module` matches `ts_library`.

Note: This is denoted as `refactor` as we currently are not able to
merge feature commits into patch branches, but we want the tooling
to not diverge significantly between the patch and next branch. It is
planned to update the merge tooling to allow for such changes to land.
Updates the Bazel NodeJS rules to v4.0.0-beta.0. This is necessary
so that the Angular components repo can update, and it's generally
good to stay as up-to-date as possible with the Bazel rules as it's
easy to fall behind, and updating early allows us to discover issues
affecting our tooling earlier (where they are easier to address due to
e.g. potential breaking change policy).
@devversion devversion force-pushed the build/update-rules-nodejs-v4-beta branch from 73d29ac to ffd833b Jul 9, 2021
@AndrewKushnir
Copy link
Contributor

@AndrewKushnir AndrewKushnir commented Jul 9, 2021

Loading

AndrewKushnir added a commit that referenced this issue Jul 9, 2021
We removed `bazel-toolchains` from the repository since dev-infra
provides the RBE platforms now (in a way where they are not reliant
on the Bazel version), so the comment in `.bazelversion` can be
removed.

PR Close #42760
AndrewKushnir added a commit that referenced this issue Jul 9, 2021
Skydoc is no longer used as `@angular/bazel` is no longer a
public API. The Sass rules were only used in a single place
in the repo where Sass is not really needed and has just been
added by accident most likely. We want to remove the Sass dependency
in preparation for Rules NodeJS v4.x where the Sass rules currently
still use an older version of `@bazel/worker` that is incompatible.

PR Close #42760
AndrewKushnir added a commit that referenced this issue Jul 9, 2021
This commit applies changes to `@angular/bazel` which are necessary
to support the Bazel NodeJS rules v4.0.0. The Bazel NodeJS rules
no longer support the `_tslibrary` option for the `LinkablePackageInfo`
provider and therefore we need to stop using it. Due to this removal,
we also need to add two new attributes called `package_name` and
`package_path` so that the API of `ng_module` matches `ts_library`.

Note: This is denoted as `refactor` as we currently are not able to
merge feature commits into patch branches, but we want the tooling
to not diverge significantly between the patch and next branch. It is
planned to update the merge tooling to allow for such changes to land.

PR Close #42760
AndrewKushnir added a commit that referenced this issue Jul 9, 2021
Updates the Bazel NodeJS rules to v4.0.0-beta.0. This is necessary
so that the Angular components repo can update, and it's generally
good to stay as up-to-date as possible with the Bazel rules as it's
easy to fall behind, and updating early allows us to discover issues
affecting our tooling earlier (where they are easier to address due to
e.g. potential breaking change policy).

PR Close #42760
@AndrewKushnir
Copy link
Contributor

@AndrewKushnir AndrewKushnir commented Jul 9, 2021

@devversion FYI this PR is now merged into the master branch, but there was a marge conflict with the patch branch (so the changes were not merged to the patch branch). Could you please create a followup PR with patch-only changes if needed? Thank you.

Loading

@angular-automatic-lock-bot
Copy link

@angular-automatic-lock-bot angular-automatic-lock-bot bot commented Aug 10, 2021

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.

Loading

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Aug 10, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.