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
build: update to rules_nodejs
v4.0.0-beta.0
#42760
Conversation
20698d2
to
db04451
rules_nodejs
v4.0.0-beta.0rules_nodejs
v4.0.0-beta.0
f36c349
to
ddb9c93
@@ -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", |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
ddb9c93
to
0268ab9
Disabling pull approve since all the remaining reviews are needed for basic |
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. |
2ef0fa8
to
64119ac
Thanks @AndrewKushnir, sounds good. Note that I updated commit changing |
@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. |
64119ac
to
73d29ac
@AndrewKushnir Done! |
… 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).
73d29ac
to
ffd833b
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
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
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
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
@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. |
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
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).
The text was updated successfully, but these errors were encountered: