bazel: upgrade to rules_nodejs 5.4.2

[sjbarag]

Please forgive the massive second commit — there's very few valid states in this progression, as building, linting, and testing either work or they don't. There's not much sense in intentionally leaving commits around that won't build in my opinion, as it makes bisecting extremely frustrating. If anyone disagrees, let me know and I can keep digging for an intermediate state!

---

Upgrading to Bazel's rules_nodejs 5.x exposed a flaw in our previous Bazel integration: because rules_nodejs explicitly doesn't support yarn's "workspaces" feature [1] (in which common dependencies are hoisted to the lowest common parent directory), any NPM dependencies with different major versions between db-console and cluster-ui would
get flattened to a single version. This left one of those packages using an unsupported (and un-requested) version of a dependency. Removing the yarn workspace layout and using separate Bazel repositories for each JS project ensured each project received the correct dependencies, but revealed incompatibilities with the requested versions. Upgrade rules_nodejs to the latest released version, remove yarn workspaces from the pkg/ui/ tree, and fix all revealed compatibility issues.

[cockroach-teamcity]

This change is 

[sjbarag]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @rickystewart)

---

pkg/cmd/dev/build.go line 139 at r2 (raw file):

	if cross == "" {
		// run "yarn --check-files" before any bazel target that includes UI to ensure that node_modules dir is consistent

This should be happening automatically now that we're seeding the global cache and running yarn install based on that cache, but if that's not accurate let me know!

Code quote:

		// run "yarn --check-files" before any bazel target that includes UI to ensure that node_modules dir is consistent
		// see related issue: https://github.com/cockroachdb/cockroach/issues/70867
		for _, arg := range args {
			if arg == "--config=with_ui" {
				logCommand("bazel", "run", "@nodejs//:yarn", "--", "--check-files", "--cwd", "pkg/ui", "--offline")
				if err := d.exec.CommandContextInheritingStdStreams(ctx, "bazel", "run", "@nodejs//:yarn", "--", "--check-files", "--cwd", "pkg/ui", "--offline"); err != nil {
					return err
				}
				break
			}
		}

---

pkg/ui/patches/aria-query/remove-filenames-with-spaces.cluster-ui.patch line 1 at r2 (raw file):

diff -Naur "pkg/ui/workspaces/cluster-ui/node_modules/aria-query/lib/etc/roles/abstract/commandRole 2.js" "pkg-no-spaces/ui/workspaces/cluster-ui/node_modules/aria-query/lib/etc/roles/abstract/commandRole 2.js"

aria-query@5.0.0 includes files in its .tgz package that have spaces in their filenames. Bazel breaks when it encounters files with a space in their name, and 5.0.0 is the latest version of aria-query. Committing a version of aria-query-5.0.0.tgz in yarn-vendor without the offending files is ineffective, unfortunately, as the next yarn install overwrites the modified copy with a copy including spaced files again.

---

pkg/ui/patches/aria-query/remove-filenames-with-spaces.db-console.patch line 1 at r2 (raw file):

diff -Naur "pkg/ui/workspaces/db-console/node_modules/aria-query/lib/etc/roles/abstract/commandRole 2.js" "pkg-no-spaces/ui/workspaces/db-console/node_modules/aria-query/lib/etc/roles/abstract/commandRole 2.js"

Same as the one affecting cluster-ui. Since we have separate Bazel repositories for each JS package, we need to apply the patch twice. And since these patches are applied from the root of the Bazel workspace, we can't -p our way around it.

We could perhaps use --directory I suppose, but that's unclear to me at the moment.

---

pkg/ui/workspaces/cluster-ui/babel.config.js line 38 at r2 (raw file):

  test: {
    plugins: [
      [ "@babel/plugin-transform-runtime", { absoluteRuntime: true } ],

Required to make Babel'd imports work with Bazel's sandbox during testing

---

pkg/ui/workspaces/db-console/package.json line 24 at r2 (raw file):

    "@cockroachlabs/design-tokens": "0.4.5",
    "@cockroachlabs/icons": "0.3.0",
    "@cockroachlabs/ui-components": "0.2.20",

This was included via cluster-ui previously, but since we're not using yarn workspaces anymore we need to manually specify it

---

pkg/ui/workspaces/db-console/package.json line 142 at r2 (raw file):

    "mocha": "^6.2.1",
    "nib": "^1.1.2",
    "prettier": "^1.19.1",

Bazel always gave db-console prettier@1.x instead of 2.x like it requested. To avoid linting failures caused by the sudden upgrade, just downgrade prettier.

[rickystewart]

Nit right off the bat: the PR message/commit title says "4.5.2", but you mean "5.4.2".

[sjbarag]

Nit right off the bat: the PR message/commit title says "4.5.2", but you mean "5.4.2".

Clearly I meant 2.4.4.4.4.5.2. Good catch 😅

[sjbarag]

(rebased on 2c1ac2d875 (Merge #81396, 2022-05-18))

[rickystewart]

Looking good overall.

Looks like the make-based build is failing:

make: *** [Makefile:1328: pkg/ui/workspaces/db-console/src/js/protos.js] Error 127
make: *** Deleting file 'pkg/ui/workspaces/db-console/src/js/protos.js'
make: *** Waiting for unfinished jobs....
make: *** [Makefile:1322: pkg/ui/workspaces/db-console/ccl/src/js/protos.js] Error 127
make: *** Deleting file 'pkg/ui/workspaces/db-console/ccl/src/js/protos.js'
build/node-run.sh: line 51: pkg/ui/node_modules/.bin/pbjs: No such file or directory
build/node-run.sh: line 51: pkg/ui/node_modules/.bin/pbjs: No such file or directory

Unfortunately we have to keep the make build working for now. LMK if you need any help with this stuff.

You also need to dev test pkg/cmd/dev --rewrite.

[rickystewart]

Please replace this with the mirrored URL: https://storage.googleapis.com/public-bazel-artifacts/bazel/rules_nodejs-5.4.2.tar.gz

[rickystewart]

This change is causing the generated code check to fail with "did not parse all needed data from node_repositories call". I'll need to patch pkg/cmd/generate-distdir to deal with this, give me a second.

[rickystewart]

Try the following patch please and dev generate bazel. I haven't tested this, it might need more tweaking, let me know if this doesn't work.

diff --git a/pkg/cmd/generate-distdir/main.go b/pkg/cmd/generate-distdir/main.go
index 5b5f5c80e9..ef66b7d5d5 100644
--- a/pkg/cmd/generate-distdir/main.go
+++ b/pkg/cmd/generate-distdir/main.go
@@ -111,7 +111,7 @@ func getShasFromGoDownloadSdkCall(call *syntax.CallExpr, shas map[string]string)
 }
 
 func getShasFromNodeRepositoriesCall(call *syntax.CallExpr, shas map[string]string) error {
-	var nodeURLTmpl, yarnURLTmpl, nodeVersion, yarnVersion, yarnSha, yarnFilename string
+	var nodeURLTmpl, nodeVersion string
 	nodeBaseToHash := make(map[string]string)
 	for _, arg := range call.Args {
 		switch bx := arg.(type) {
@@ -151,7 +151,31 @@ func getShasFromNodeRepositoriesCall(call *syntax.CallExpr, shas map[string]stri
 				if err != nil {
 					return err
 				}
-			} else if kwarg == "yarn_repositories" {
+			}
+		}
+	}
+	if nodeURLTmpl == "" || nodeVersion == "" {
+		return fmt.Errorf("did not parse all needed data from node_repositories call")
+	}
+	for base, sha := range nodeBaseToHash {
+		shas[strings.ReplaceAll(strings.ReplaceAll(nodeURLTmpl, "{version}", nodeVersion), "{filename}", base)] = sha
+	}
+	return nil
+}
+
+func getShasFromYarnRepositoriesCall(call *syntax.CallExpr, shas map[string]string) error {
+	var yarnURLTmpl, yarnVersion, yarnSha, yarnFilename string
+	for _, arg := range call.Args {
+		switch bx := arg.(type) {
+		case *syntax.BinaryExpr:
+			if bx.Op != syntax.EQ {
+				return fmt.Errorf("unexpected binary expression Op %d", bx.Op)
+			}
+			kwarg, err := starlarkutil.ExpectIdent(bx.X)
+			if err != nil {
+				return err
+			}
+			if kwarg == "yarn_releases" {
 				switch d := bx.Y.(type) {
 				case *syntax.DictExpr:
 					if len(d.List) != 1 {
@@ -184,11 +208,8 @@ func getShasFromNodeRepositoriesCall(call *syntax.CallExpr, shas map[string]stri
 			}
 		}
 	}
-	if nodeURLTmpl == "" || yarnURLTmpl == "" || nodeVersion == "" || yarnVersion == "" || yarnSha == "" || yarnFilename == "" {
-		return fmt.Errorf("did not parse all needed data from node_repositories call")
-	}
-	for base, sha := range nodeBaseToHash {
-		shas[strings.ReplaceAll(strings.ReplaceAll(nodeURLTmpl, "{version}", nodeVersion), "{filename}", base)] = sha
+	if yarnURLTmpl == "" || yarnVersion == "" || yarnSha == "" || yarnFilename == "" {	
+		return fmt.Errorf("did not parse all needed data from yarn_repositories call")
 	}
 	shas[strings.ReplaceAll(strings.ReplaceAll(yarnURLTmpl, "{version}", yarnVersion), "{filename}", yarnFilename)] = yarnSha
 	return nil
@@ -227,6 +248,11 @@ func getShasFromWorkspace(workspace string, shas map[string]string) error {
 						return err
 					}
 				}
+				if fun == "yarn_repositories" {
+					if err := getShasFromYarnRepositoriesCall(x, shas); err != nil {
+						return err
+					}
+				}
 
 			}
 		}

[sjbarag]

./dev generate bazel worked with that patch 🎉 Thanks @rickystewart !

[rickystewart]

Nice!

[rickystewart]

Nit: It seems like the 3 files you're exposing from this yarn_cache repo don't actually have separate meanings -- whether you depend on db_console.seed or cluster_ui.seed or whatever doesn't make a difference. You might as well have one file called .seed. OR you can have 3 different repositories, each of which seeds one directory (@yarn_seed_db_console//:.seed, @yarn_seed_cluster_ui//:.seed), and then which you choose for the data dependency would be meaningful. The current in-between state is kind of confusing though.

[sjbarag]

Good catch! I was hoping to keep the various seeds isolated, so that building cluster-ui doesn't require db-console dependencies to be seeded. Should be easy to split this repository_rule into a "one per package" kind of thing :)

[rickystewart]

Not really relevant to this PR, but do you know how we might go about auto-generating this stuff so we can keep it up to date? Reviewing these changes is pretty tough and updating them seems equally tough.

[sjbarag]

The best I can think of is using buildozer, making this a list of all the declared direct dependencies in package.json, and filtering out the few that shouldn't be there. At that point we're mixing test dependencies with build dependencies and we may as well just use @npm_cluster_ui//:node_modules, since that filegroup (or is it a depset?) is pre-defined by rules_nodejs.

The only other thing we could do is something like "use the TypeScript Compiler API to find import statements and exclude imports starting with src/ or ccl/ or .. It'd get us the runtime dependencies, but we'd still miss all of our build tooling 😬

There's unfortunately not many good options here.

[rickystewart]

What does this do and is there a disadvantage to not validateing?

[sjbarag]

It disables a build-time check for a block like this in our tsconfig.json:

"compilerOptions": {
    "rootDirs": [
        ".",
        "../../bazel-out/host/bin/path/to",
        "../../bazel-out/darwin-fastbuild/bin/path/to",
        "../../bazel-out/darwin_arm64-fastbuild/bin/path/to",
        "../../bazel-out/k8-fastbuild/bin/path/to",
        "../../bazel-out/x64_windows-fastbuild/bin/path/to",
        "../../bazel-out/darwin-dbg/bin/path/to",
        "../../bazel-out/k8-dbg/bin/path/to",
        "../../bazel-out/x64_windows-dbg/bin/path/to",
    ]
}

(example from the rules_nodejs docs; we'd have ../../../../../../bazel-out/(the rest) because of our directory nesting)
which tells the TypeScript compiler how to find types installed in the Bazel sandbox & output directories. But since we're using the node_modules directories within pkg/ui/… and we're linking our packages together via the yarn_install, there's no need to for the typescript compiler to look elsewhere. Everything just kind of works for us without validating, so we can safely disable it.

Adding a comment now!

[rickystewart]

Try the following patch please and dev generate bazel. I haven't tested this, it might need more tweaking, let me know if this doesn't work.

diff --git a/pkg/cmd/generate-distdir/main.go b/pkg/cmd/generate-distdir/main.go
index 5b5f5c80e9..ef66b7d5d5 100644
--- a/pkg/cmd/generate-distdir/main.go
+++ b/pkg/cmd/generate-distdir/main.go
@@ -111,7 +111,7 @@ func getShasFromGoDownloadSdkCall(call *syntax.CallExpr, shas map[string]string)
 }
 
 func getShasFromNodeRepositoriesCall(call *syntax.CallExpr, shas map[string]string) error {
-	var nodeURLTmpl, yarnURLTmpl, nodeVersion, yarnVersion, yarnSha, yarnFilename string
+	var nodeURLTmpl, nodeVersion string
 	nodeBaseToHash := make(map[string]string)
 	for _, arg := range call.Args {
 		switch bx := arg.(type) {
@@ -151,7 +151,31 @@ func getShasFromNodeRepositoriesCall(call *syntax.CallExpr, shas map[string]stri
 				if err != nil {
 					return err
 				}
-			} else if kwarg == "yarn_repositories" {
+			}
+		}
+	}
+	if nodeURLTmpl == "" || nodeVersion == "" {
+		return fmt.Errorf("did not parse all needed data from node_repositories call")
+	}
+	for base, sha := range nodeBaseToHash {
+		shas[strings.ReplaceAll(strings.ReplaceAll(nodeURLTmpl, "{version}", nodeVersion), "{filename}", base)] = sha
+	}
+	return nil
+}
+
+func getShasFromYarnRepositoriesCall(call *syntax.CallExpr, shas map[string]string) error {
+	var yarnURLTmpl, yarnVersion, yarnSha, yarnFilename string
+	for _, arg := range call.Args {
+		switch bx := arg.(type) {
+		case *syntax.BinaryExpr:
+			if bx.Op != syntax.EQ {
+				return fmt.Errorf("unexpected binary expression Op %d", bx.Op)
+			}
+			kwarg, err := starlarkutil.ExpectIdent(bx.X)
+			if err != nil {
+				return err
+			}
+			if kwarg == "yarn_releases" {
 				switch d := bx.Y.(type) {
 				case *syntax.DictExpr:
 					if len(d.List) != 1 {
@@ -184,11 +208,8 @@ func getShasFromNodeRepositoriesCall(call *syntax.CallExpr, shas map[string]stri
 			}
 		}
 	}
-	if nodeURLTmpl == "" || yarnURLTmpl == "" || nodeVersion == "" || yarnVersion == "" || yarnSha == "" || yarnFilename == "" {
-		return fmt.Errorf("did not parse all needed data from node_repositories call")
-	}
-	for base, sha := range nodeBaseToHash {
-		shas[strings.ReplaceAll(strings.ReplaceAll(nodeURLTmpl, "{version}", nodeVersion), "{filename}", base)] = sha
+	if yarnURLTmpl == "" || yarnVersion == "" || yarnSha == "" || yarnFilename == "" {	
+		return fmt.Errorf("did not parse all needed data from yarn_repositories call")
 	}
 	shas[strings.ReplaceAll(strings.ReplaceAll(yarnURLTmpl, "{version}", yarnVersion), "{filename}", yarnFilename)] = yarnSha
 	return nil
@@ -227,6 +248,11 @@ func getShasFromWorkspace(workspace string, shas map[string]string) error {
 						return err
 					}
 				}
+				if fun == "yarn_repositories" {
+					if err := getShasFromYarnRepositoriesCall(x, shas); err != nil {
+						return err
+					}
+				}
 
 			}
 		}

[rickystewart]

You will also need to declare a reference to the core module. (If you don't do this, the call to build_bazel_rules_nodejs_dependencies() will pull in a non-mirrored version of the library from upstream GitHub, which is a big no-no.)

http_archive(
    name = "rules_nodejs",
    sha256 = "26766278d815a6e2c43d2f6c9c72fde3fec8729e84138ffa4dabee47edc7702a",
    urls = ["https://storage.googleapis.com/public-bazel-artifacts/bazel/rules_nodejs-core-5.4.2.tar.gz"],
)

[rickystewart]

BEFORE the call to build_bazel_rules_nodejs_dependencies(), please add the following comments:

# bazel_skylib handled above.
# rules_nodejs handled above.

This mirrors other comments we have in WORKSPACE. Basically these comments give us (the maintainers) confidence that we have manually checked the dependencies and mirrored all of them.

[rickystewart]

One more thing: AFTER addressing all of the above comments, please bazel build @distdir//:archives and make sure the build succeeds.

[sjbarag]

Make should be fixed now — looking at the remaining fixes tomorrow morning :)

[sjbarag]

I'll resolve the merge conflicts in a moment, but all issues resolved so far.

rules_nodejs 5.5.0 is now available, but I'll upgrade to that in a separate PR to prevent this one from growing too long 😃

[rickystewart]

Not sure if this was meant to be reviewed yet, but please resolve the merge conflicts, please squash/rebase the 11 commits (including many fixup!), and looks like the lint and dev_test is still not happy.

[sjbarag]

Not sure if this was meant to be reviewed yet, but please resolve the merge conflicts, please squash/rebase the 11 commits (including many fixup!), and looks like the lint and dev_test is still not happy.

Heh, yeah not ready for review yet sorry! The fixup commits are from git commit --fixup and will be git rebase -i --autosquash'd down before the next review. I'll tag ya 😄

[sjbarag]

(Reabsed on 4dc4279 (Merge #81663 #81778 #81793, 2022-05-25) ; letting CI do its thing)

[sjbarag]

@rickystewart This is ready for re-review. All commits squashed down to their final desired states. I imagine we'll see one unit test failing (from pkg/ccl/sqlproxyccl/BUILD.bazel), which isn't caused by this PR 😃

[rickystewart]

Awesome! Would be good if you got a second reviewer for the JS bits.

[sjbarag]

@laurenbarker or @dhartunian would one of y’all mind checking the JS parts for me? 🙂

[sjbarag]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @dhartunian, @laurenbarker, @nathanstilwell, @rickystewart, and @sjbarag)

---

pkg/ui/workspaces/cluster-ui/package.json line 162 at r29 (raw file):

Previously, nathanstilwell (Nathan Stilwell) wrote…

Why is this a resolution instead of just pinning the version in dev dependencies? Mostly interested for my own Jest work. 😄

TBH I can't remember 😅 let me see if Ic an remove that real fast

[sjbarag]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @dhartunian, @laurenbarker, @nathanstilwell, @rickystewart, and @sjbarag)

---

pkg/ui/workspaces/cluster-ui/package.json line 162 at r29 (raw file):

Previously, sjbarag (Sean Barag) wrote…

TBH I can't remember 😅 let me see if Ic an remove that real fast

Ah, this is because we're not using jsdom directly, but it gets pulled in through jest-environment-enzyme:

$ yarn why jest-environment-jsdom
yarn why v1.22.18
[1/4] Why do we have the module "jest-environment-jsdom"...?
[2/4] Initialising dependency graph...
warning Resolution field "protobufjs@6.8.6" is incompatible with requested version "protobufjs@6.8.8"
[3/4] Finding dependency...
[4/4] Calculating file sizes...
=> Found "jest-environment-jsdom@27.5.1"
info Reasons this module exists
   - "jest-environment-enzyme" depends on it
   - Hoisted from "jest-environment-enzyme#jest-environment-jsdom"
   - Hoisted from "jest-cli#jest-config#jest-environment-jsdom"
   - Hoisted from "jest#@jest#core#jest-runner#jest-environment-jsdom"
info Disk size without dependencies: "960KB"
info Disk size with unique dependencies: "11.2MB"
info Disk size with transitive dependencies: "29.07MB"
info Number of shared dependencies: 77
Done in 1.00s.

Unfortunately, we're using an older version of jest-environment-enzyme (7.1.2) which declares a dependency on jest-environment-jsdom@^24.0.0. IIRC the jest-environment-jsdom version must match the version of Jest used, but since we don't directly depend on jest-environment-jsdom there's no need to add it to devDependencies 😃

[sjbarag]

Remaining maintenance tasks:

• Merge branch sjbarag/upgrade-to-rules_nodejs-542 of https://github.com/cockroachdb/yarn-vendored into master
• Update the git submodule at pkg/ui/yarn-vendor in this PR if there's any sha differences
• Rebase this PR on origin/master once again to ensure the test timeout is resolved.
• Address @laurenbarker's comments 🙂

[laurenbarker]

Thanks for taking this on @sjbarag! I mostly had questions :)

Do you think it's worth removing or renaming the workspaces directory so developers don't assume we are using yarn workspaces?

Reviewed 1 of 30 files at r28, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @dhartunian, @nathanstilwell, @rickystewart, and @sjbarag)

---

-- commits line 27 at r29:
Wow, this issue thread is a journey. Thanks for referencing the link.

---

WORKSPACE line 247 at r29 (raw file):

# repositories, to avoid version conflicts between those packages.
# Unfortunately Bazel's rules_nodejs does not support yarn workspaces, so 
# packages have isolated dependencies and must be installed as isolated

If we start using Renovate to keep JS deps up-to-date in this repo, it will help us keep our dependency versions consistent across packages. I don't think we have enough packages to need something like SyncPack (mentioned in bazel-contrib/rules_nodejs#266 (comment)).

---

WORKSPACE line 257 at r29 (raw file):

    data = [
      "//pkg/ui:.yarnrc",
      "@yarn_cache//:.seed",

I'm curious how this works. Do you happen to have a link to any relevant docs? Jk, I came across the script you wrote later in the file tree.

---

WORKSPACE line 282 at r29 (raw file):

      "//pkg/ui:patches/aria-query/remove-filenames-with-spaces.db-console.patch",
    ],
    symlink_node_modules = True,

Is it possible to add comments explaining what each of these options is doing or a link to docs that explain what these are doing? If I needed to edit this file, I wouldn't know which docs to look at (i.e., rules_nodejs, yarn, bazel). Although, perhaps we have this information is a README somewhere and I'm just not aware of it since I haven't been following the bazel work.

---

build/bazelutil/seed_yarn_cache.bzl line 14 at r29 (raw file):

    )

    rctx.report_progress("Checking for yarn-vendor submodule...")

Ah, so is the yarn cache just the yarn-vendor submodule? Am I understanding this correctly?

---

pkg/ui/not-yarn-workspace.sh line 17 at r29 (raw file):

dependencies, be sure to `cd` into the correct package within workspaces/.

As a convenience, running `yarn install` from pkg/ui/ still installs the

Do we want to support this? How common is it to need to install a dep in all packages? My worry is that devs run yarn install in the wrong directory all the time and could end up adding dependencies to the wrong packages on accident.

[sjbarag]

Thanks for slogging through this, y'all!

Do you think it's worth removing or renaming the workspaces directory so developers don't assume we are using yarn workspaces?

I considered it but didn't want to bloat this PR with even more stuff. As a separate PR it's probably worth doing, though I'm not sure what I'd call it. pkg/ui/packages/… feels like it might imply Lerna? 🤔

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @dhartunian, @laurenbarker, @nathanstilwell, @rickystewart, and @sjbarag)

---

WORKSPACE line 32 at r3 (raw file):

Previously, rickystewart (Ricky Stewart) wrote…

Please replace this with the mirrored URL: https://storage.googleapis.com/public-bazel-artifacts/bazel/rules_nodejs-5.4.2.tar.gz

Done.

---

WORKSPACE line 34 at r3 (raw file):

Previously, rickystewart (Ricky Stewart) wrote…

You will also need to declare a reference to the core module. (If you don't do this, the call to build_bazel_rules_nodejs_dependencies() will pull in a non-mirrored version of the library from upstream GitHub, which is a big no-no.)

http_archive(
    name = "rules_nodejs",
    sha256 = "26766278d815a6e2c43d2f6c9c72fde3fec8729e84138ffa4dabee47edc7702a",
    urls = ["https://storage.googleapis.com/public-bazel-artifacts/bazel/rules_nodejs-core-5.4.2.tar.gz"],
)

Done.

---

WORKSPACE line 201 at r3 (raw file):

Previously, rickystewart (Ricky Stewart) wrote…

BEFORE the call to build_bazel_rules_nodejs_dependencies(), please add the following comments:

# bazel_skylib handled above.
# rules_nodejs handled above.

This mirrors other comments we have in WORKSPACE. Basically these comments give us (the maintainers) confidence that we have manually checked the dependencies and mirrored all of them.

Done.

---

WORKSPACE line 247 at r29 (raw file):

Previously, laurenbarker (Lauren Barker) wrote…

If we start using Renovate to keep JS deps up-to-date in this repo, it will help us keep our dependency versions consistent across packages. I don't think we have enough packages to need something like SyncPack (mentioned in bazel-contrib/rules_nodejs#266 (comment)).

Good call! SyncPack definitely isn't necessary right now. As long as Renovate supports having multiple package.json and multiple yarn.lock files in a single repo, I'm fine with adding Renovate here!

---

WORKSPACE line 257 at r29 (raw file):

Previously, laurenbarker (Lauren Barker) wrote…

I'm curious how this works. Do you happen to have a link to any relevant docs? Jk, I came across the script you wrote later in the file tree.

Done.

---

WORKSPACE line 282 at r29 (raw file):

Previously, laurenbarker (Lauren Barker) wrote…

Is it possible to add comments explaining what each of these options is doing or a link to docs that explain what these are doing? If I needed to edit this file, I wouldn't know which docs to look at (i.e., rules_nodejs, yarn, bazel). Although, perhaps we have this information is a README somewhere and I'm just not aware of it since I haven't been following the bazel work.

I don't believe it's listed in a README anywhere. :/ These all come from https://bazelbuild.github.io/rules_nodejs/Built-ins.html#yarn_install , so I can add some comments here.

---

build/bazelutil/seed_yarn_cache.bzl line 14 at r29 (raw file):

Previously, laurenbarker (Lauren Barker) wrote…

Ah, so is the yarn cache just the yarn-vendor submodule? Am I understanding this correctly?

You're close! There's two phases here, and I've gotta clean up the comments in this file (the comment for _seed_yarn_cache_impl is completely wrong anyway 😓

Bazel's yarn_install doesn't have access to the yarn-vendor submodule when it's installing packages into the repo-specific directories in the Bazel output area (…/npm_db_console/, …/npm_cluster_ui, …/npm_protos), so the yarn install --offline that we do to ensure we only use vendored dependencies fails. Fixing that would require copying each .tgz file in that submodule into each of those repos, which is how I accidentally made the yarn_install process take ~50 minutes a few weeks ago 😬

The "first phase" is just checking to see if the yarn-vendor submodule exists. Without doing this, there's a chance the next phase will error out with some really weird messages.

The "second phase" is basically doing a throwaway yarn install. The yarn_install Bazel rule has access to the current user's "global" yarn cache (in ~/.cache/yarn/…), which helps to accelerate the yarn_install rule under normal situations. The seed_yarn_cache rule defined in this file ensures the ~/.cache/yarn/ directory is properly set up by doing a throwaway yarn install --offline outside of the Bazel sandbox (where we can get to yarn-vendor!) and putting the installed modules in a place we'll never read from. When the regular yarn_install rule starts, the user's "global" yarn cache is already set up so all of our dependencies are served from there. That way, it doesn't matter if yarn_install can't get to the yarn-vendor submodule at all!

It's a terrible, terrible hack. Unfortunately we can't skip the default yarn_install phase, because that's what defines the @npm_cluster_ui// packages that we reference in BUILD.bazel 😢

Maintaining our own npm registry would make this entire thing unnecessary. There's no rule against fetching dependencies from The Internet, as long as we control the hosting of those artifacts 😃

---

pkg/ui/not-yarn-workspace.sh line 17 at r29 (raw file):

Previously, laurenbarker (Lauren Barker) wrote…

Do we want to support this? How common is it to need to install a dep in all packages? My worry is that devs run yarn install in the wrong directory all the time and could end up adding dependencies to the wrong packages on accident.

Luckily, this doesn't apply to adding, removing, or updating dependencies in any of our packages! This only installs the packages that are already declared (yarn with no arguments is an alias for yarn install). We could easily remove this file and the postinstall script that calls it, but it'd force people to have to cd into a few different directories and run yarn on their own (…or just let Bazel do it I guess?)

[rickystewart]

Maintaining our own npm registry would make this entire thing unnecessary. There's no rule against fetching dependencies from The Internet, as long as we control the hosting of those artifacts 😃

Yes and no, we do still want to maintain support for offline builds. The stuff doesn't necessarily have to be vendored in-tree though. As long as we provide a path for downloading the UI dependencies ahead of time such that you don't always have to hit the internet, this works.

[laurenbarker]

I considered it but didn't want to bloat this PR with even more stuff. As a separate PR it's probably worth doing, though I'm not sure what I'd call it. pkg/ui/packages/… feels like it might imply Lerna?

/packages (and being done is a separate PR) makes sense to me! I don't think devs would assume Lerna since it would be obvious from package.json that lerna is not a dep and we have no scripts for lerna commands.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @dhartunian, @laurenbarker, @nathanstilwell, @rickystewart, and @sjbarag)

---

WORKSPACE line 247 at r29 (raw file):

Previously, sjbarag (Sean Barag) wrote…

Good call! SyncPack definitely isn't necessary right now. As long as Renovate supports having multiple package.json and multiple yarn.lock files in a single repo, I'm fine with adding Renovate here!

Renovate does support multiple packages in one repo. We currently use renovate in cockroachdb/ui and it's worked well so far!

---

WORKSPACE line 282 at r29 (raw file):

Previously, sjbarag (Sean Barag) wrote…

I don't believe it's listed in a README anywhere. :/ These all come from https://bazelbuild.github.io/rules_nodejs/Built-ins.html#yarn_install , so I can add some comments here.

Ah, that info is very helpful! Thanks!

---

build/bazelutil/seed_yarn_cache.bzl line 14 at r29 (raw file):

Previously, sjbarag (Sean Barag) wrote…

You're close! There's two phases here, and I've gotta clean up the comments in this file (the comment for _seed_yarn_cache_impl is completely wrong anyway 😓

Bazel's yarn_install doesn't have access to the yarn-vendor submodule when it's installing packages into the repo-specific directories in the Bazel output area (…/npm_db_console/, …/npm_cluster_ui, …/npm_protos), so the yarn install --offline that we do to ensure we only use vendored dependencies fails. Fixing that would require copying each .tgz file in that submodule into each of those repos, which is how I accidentally made the yarn_install process take ~50 minutes a few weeks ago 😬

The "first phase" is just checking to see if the yarn-vendor submodule exists. Without doing this, there's a chance the next phase will error out with some really weird messages.

The "second phase" is basically doing a throwaway yarn install. The yarn_install Bazel rule has access to the current user's "global" yarn cache (in ~/.cache/yarn/…), which helps to accelerate the yarn_install rule under normal situations. The seed_yarn_cache rule defined in this file ensures the ~/.cache/yarn/ directory is properly set up by doing a throwaway yarn install --offline outside of the Bazel sandbox (where we can get to yarn-vendor!) and putting the installed modules in a place we'll never read from. When the regular yarn_install rule starts, the user's "global" yarn cache is already set up so all of our dependencies are served from there. That way, it doesn't matter if yarn_install can't get to the yarn-vendor submodule at all!

It's a terrible, terrible hack. Unfortunately we can't skip the default yarn_install phase, because that's what defines the @npm_cluster_ui// packages that we reference in BUILD.bazel 😢

Maintaining our own npm registry would make this entire thing unnecessary. There's no rule against fetching dependencies from The Internet, as long as we control the hosting of those artifacts 😃

Ohhh, wow, I would not say I was close, lol. Thanks for the explanation!

---

pkg/ui/not-yarn-workspace.sh line 17 at r29 (raw file):

Previously, sjbarag (Sean Barag) wrote…

Luckily, this doesn't apply to adding, removing, or updating dependencies in any of our packages! This only installs the packages that are already declared (yarn with no arguments is an alias for yarn install). We could easily remove this file and the postinstall script that calls it, but it'd force people to have to cd into a few different directories and run yarn on their own (…or just let Bazel do it I guess?)

Oh, I see. I definitely did interpret this file correctly. I was thinking about adding new deps. I don't see any harm in making installs easier!

[sjbarag]

Maintaining our own npm registry would make this entire thing unnecessary. There's no rule against fetching dependencies from The Internet, as long as we control the hosting of those artifacts 😃

Yes and no, we do still want to maintain support for offline builds. The stuff doesn't necessarily have to be vendored in-tree though. As long as we provide a path for downloading the UI dependencies ahead of time such that you don't always have to hit the internet, this works.

Great point! This PR still hooks into the bazel fetch paradigm (since it's a repository_rule), and using a CRL-managed registry would do the same :)

[sjbarag]

bors r=rickystewart,nathanstilwell,laurenbarker

[craig]

Build succeeded:

• GitHub CI (Cockroach)