-
Notifications
You must be signed in to change notification settings - Fork 980
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
chore: add basic bazel support #465
Conversation
|
||
# Setup the Node.js toolchain | ||
node_repositories( | ||
node_version = "15.0.0", |
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.
We're currently on node v12 in production, and the majority of packages use the < v14 / npm6 lockfile.
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.
latest LTE is 14 so maybe we go w/ that
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.
👍
WORKSPACE
Outdated
# TODO(npm): make sure that the package.json in individual packages/*/package.json have | ||
# consistent versions from the global one. This will most likely have to be don | ||
# by having those files generated from the main package.json. | ||
# |
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.
This would be nice one day 😄
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.
Right, so the decision to make is:
- do we want to centrally manage the versions?
- or do we want to have sub-packages in each folder?
(see my other comment)
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.
I went down path 1 and I ran into issues. So I think for now path 2 has less resistance. Let's do that for now.
package.json
Outdated
@@ -1,15 +1,38 @@ | |||
{ | |||
"private": true, | |||
"devDependencies": { | |||
"@angular/common": "^11.2.13", |
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.
Are the angular dependencies supposed to be pulled up onto the root package.json? Or are these hoisted from packages/angular?
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.
Right, so this is one of the discussions we should have.
There are two schools of thought on this one, and both have pros/cons:
- In mono-repo you should have a one-version policy and there fore you should have one place to manage all of your versions etc... The downside is that you will need to have a way to make package.json for individual sub-packages. This is what Angular uses.
- The other option is to have a mano-repo, but really have a different version for each sub-package. Which I think is what you are suggesting.
A generously don't know what the right answer is here. I have discussed this for a bit with @steve8708 and we decided to go the first route, but I think it is a bit riskier. But would love to know what other opinions are.
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.
Personally not advocating for one over the other at the moment.
I think that option one (mono-repo) is the ideal.
The downside is that you will need to have a way to make package.json for individual sub-packages
Does rules_nodejs, or something else, have a mechanism to support this, or would this be a manual task right now?
If I understand the semantics of packages/angular/BUILD, when building with Bazel it handles adding the listed deps
to the Node path, is that correct?
builder/packages/angular/BUILD
Lines 15 to 26 in 64f517e
"//packages/core", | |
"@npm//@angular/common", | |
"@npm//@angular/core", | |
"@npm//@angular/elements", | |
"@npm//@angular/platform-browser", | |
"@npm//@angular/platform-browser-dynamic", | |
"@npm//@angular/platform-server", | |
"@npm//@angular/router", | |
"@npm//@nguniversal/express-engine", | |
"@npm//@nguniversal/module-map-ngfactory-loader", | |
"@npm//@types/express", | |
"@npm//express", |
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.
ah, I think I misunderstood something from our last convo that makes me think we actually want mano-repo
my thought process was that we want all internal dependencies to be locked to the same version (e.g. every SDK that relies on @builder.io/core should use the latest version of that always)
if mono-repo means that every SDK must use the same version (e.g. if we want to just update one SDK, like webcomponents, we have to update every other SDK to that same version - e.g. 3.0.0 or whatnot), that I don't think we want bc not every SDK related to every other one
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.
Mono-repo does not really mean anything. It is whatever you define it to be.
Yes I went down path 1 where the versions are shared between packages but ran into an issue, so I am abandoning it and going down path 2 which is what you are advocating here.
Awesome! Can't wait to have a more well managed repo :) |
packages/angular/ng-package.json
Outdated
@@ -1,5 +1,6 @@ | |||
{ | |||
"$schema": "./node_modules/ng-packagr/ng-package.schema.json", | |||
"dest": "../../bazel-out/darwin-fastbuild/bin/packages/angular/ng-pkg", |
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.
This is a hack and needs to be properly fixed. Still discussing with @alexeagle on how best to do this.
Description
Adds basic Bazel support to get the discussion going.