Migrate from rules_nodejs to rules_js/rules_ts #7923

philsc · 2023-04-25T05:31:18Z

This patch migrates the current use of rules_nodejs to the new rules_js.
rules_js is the intended replacement of rules_nodejs as per this note:
https://github.com/aspect-build/rules_js#relationship-to-rules_nodejs

rules_js is an alternative to the build_bazel_rules_nodejs Bazel module
and accompanying npm packages hosted in
https://github.com/bazelbuild/rules_nodejs, which is now
unmaintained. All users are recommended to use rules_js instead.

There are a few notable changes in this patch:

The flatbuffer_ts_library macro no longer accepts a package_name
attribute. This is because rules_js appears to manage the import
naming of dependencies via top-level npm_link_package targets.
Users will have to migrate.
I added a few more arguments to flatbuffer_library_public(). These
helped with exposing esbuild to ts/compile_flat_file.sh.
I pinned the version of typescript in package.json so that
rules_ts can download the exact same version. rules_ts doesn't know
what to do if the version isn't exact.
Since rules_js uses the pnpm locking mechanism, we now have a
pnpm-lock.yaml file instead of a yarn lock file.
I added bazel targets for a few of the existing tests in tests/ts.
They can be run with bazel test //test/ts:all. Since there is no
flexbuffers bazel target, I did not add a bazel target for the
corresponding test.
I added a separate workspace in tests/ts/bazel_repository_test_dir/
to validate that the flatbuffers code can be imported as an external
repository. You can run the test with
bazel test //test/ts:bazel_repository_test. For this to work, I
needed to expose a non-trivial chunk of the flatbuffers code to the
test. I achieved this through some recursive distribution
filegroups. This is inspired by rules_python's workspace tests.

I did not do anything special to validate that the gen_reflections
parameter works the same. This patch doesn't change anything about
the TypeScript generation.

As a side note: I am not an expert with rules_js. This patch is my
attempt based on my limited understanding of the rule set.

Fixes #7817

$ bazel build //tests/ts/...

jkuszmaul

LGTM

dbaileychess · 2023-04-28T16:39:00Z

All good on this @philsc ?

philsc · 2023-04-28T18:12:21Z

Yeah, this is good from my end 👍

This reverts commit 4172c3f.

dbaileychess · 2023-04-28T19:59:03Z

Breaking buildkite: https://buildkite.com/bazel/flatbuffers/builds/8356#0187c916-2678-41fc-b62f-582ca54dc07d

philsc · 2023-04-29T00:34:16Z

Should be an easy fix.

@dbaileychess , if I upload another patch, what would it take to preserve the PR summary as the commit message? It looks like the previous merge discarded the Summary and instead listed all the useless commit message in bullet form.

This is the second version of patch google#7923. The first version got reverted because bazel query was failing: $ bazel --nosystem_rc --nohome_rc query tests(set('//...')) except tests(attr("tags", "manual", set('//...'))) ERROR: Traceback (most recent call last): File "/workdir/tests/ts/bazel_repository_test_dir/BUILD", line 6, column 22, in <toplevel> npm_link_all_packages(name = "node_modules") File "/var/lib/buildkite-agent/.cache/bazel/_bazel_buildkite-agent/ec321eb2cc2d0f8f91b676b6d4c66c29/external/npm/defs.bzl", line 188, column 13, in npm_link_all_packages fail(msg) Error in fail: The npm_link_all_packages() macro loaded from @npm//:defs.bzl and called in bazel package 'tests/ts/bazel_repository_test_dir' may only be called in bazel packages that correspond to the pnpm root package '' and pnpm workspace projects '' This was happening because the `.bazelrc` file only added `--deleted_packages` to the `build` command. We also need it for the `query` command. This second version of the patch fixes that. Original commit message: This patch migrates the current use of rules_nodejs to the new rules_js. rules_js is the intended replacement of rules_nodejs as per this note: https://github.com/aspect-build/rules_js#relationship-to-rules_nodejs > rules_js is an alternative to the build_bazel_rules_nodejs Bazel module > and accompanying npm packages hosted in > https://github.com/bazelbuild/rules_nodejs, which is now > unmaintained. All users are recommended to use rules_js instead. There are a few notable changes in this patch: 1. The `flatbuffer_ts_library` macro no longer accepts a `package_name` attribute. This is because rules_js appears to manage the import naming of dependencies via top-level `npm_link_package` targets. Users will have to migrate. 2. I added a few more arguments to `flatbuffer_library_public()`. These helped with exposing esbuild to `ts/compile_flat_file.sh`. 3. I pinned the version of `typescript` in `package.json` so that rules_ts can download the exact same version. rules_ts doesn't know what to do if the version isn't exact. 4. Since rules_js uses the pnpm locking mechanism, we now have a `pnpm-lock.yaml` file instead of a yarn lock file. 4. I added bazel targets for a few of the existing tests in `tests/ts`. They can be run with `bazel test //test/ts:all`. Since there is no flexbuffers bazel target, I did not add a bazel target for the corresponding test. 5. I added a separate workspace in `tests/ts/bazel_repository_test_dir/` to validate that the flatbuffers code can be imported as an external repository. You can run the test with `bazel test //test/ts:bazel_repository_test`. For this to work, I needed to expose a non-trivial chunk of the flatbuffers code to the test. I achieved this through some recursive `distribution` filegroups. This is inspired by rules_python's workspace tests. I did not do anything special to validate that the `gen_reflections` parameter works the same. This patch doesn't change anything about the TypeScript generation. As a side note: I am not an expert with rules_js. This patch is my attempt based on my limited understanding of the rule set. Fixes google#7817

philsc · 2023-04-29T04:45:09Z

New version up: #7928

* Migrate from rules_nodejs to rules_js/rules_ts (take 2) This is the second version of patch #7923. The first version got reverted because bazel query was failing: $ bazel --nosystem_rc --nohome_rc query tests(set('//...')) except tests(attr("tags", "manual", set('//...'))) ERROR: Traceback (most recent call last): File "/workdir/tests/ts/bazel_repository_test_dir/BUILD", line 6, column 22, in <toplevel> npm_link_all_packages(name = "node_modules") File "/var/lib/buildkite-agent/.cache/bazel/_bazel_buildkite-agent/ec321eb2cc2d0f8f91b676b6d4c66c29/external/npm/defs.bzl", line 188, column 13, in npm_link_all_packages fail(msg) Error in fail: The npm_link_all_packages() macro loaded from @npm//:defs.bzl and called in bazel package 'tests/ts/bazel_repository_test_dir' may only be called in bazel packages that correspond to the pnpm root package '' and pnpm workspace projects '' This was happening because the `.bazelrc` file only added `--deleted_packages` to the `build` command. We also need it for the `query` command. This second version of the patch fixes that. Original commit message: This patch migrates the current use of rules_nodejs to the new rules_js. rules_js is the intended replacement of rules_nodejs as per this note: https://github.com/aspect-build/rules_js#relationship-to-rules_nodejs > rules_js is an alternative to the build_bazel_rules_nodejs Bazel module > and accompanying npm packages hosted in > https://github.com/bazelbuild/rules_nodejs, which is now > unmaintained. All users are recommended to use rules_js instead. There are a few notable changes in this patch: 1. The `flatbuffer_ts_library` macro no longer accepts a `package_name` attribute. This is because rules_js appears to manage the import naming of dependencies via top-level `npm_link_package` targets. Users will have to migrate. 2. I added a few more arguments to `flatbuffer_library_public()`. These helped with exposing esbuild to `ts/compile_flat_file.sh`. 3. I pinned the version of `typescript` in `package.json` so that rules_ts can download the exact same version. rules_ts doesn't know what to do if the version isn't exact. 4. Since rules_js uses the pnpm locking mechanism, we now have a `pnpm-lock.yaml` file instead of a yarn lock file. 4. I added bazel targets for a few of the existing tests in `tests/ts`. They can be run with `bazel test //test/ts:all`. Since there is no flexbuffers bazel target, I did not add a bazel target for the corresponding test. 5. I added a separate workspace in `tests/ts/bazel_repository_test_dir/` to validate that the flatbuffers code can be imported as an external repository. You can run the test with `bazel test //test/ts:bazel_repository_test`. For this to work, I needed to expose a non-trivial chunk of the flatbuffers code to the test. I achieved this through some recursive `distribution` filegroups. This is inspired by rules_python's workspace tests. I did not do anything special to validate that the `gen_reflections` parameter works the same. This patch doesn't change anything about the TypeScript generation. As a side note: I am not an expert with rules_js. This patch is my attempt based on my limited understanding of the rule set. Fixes #7817 * Fix the query --------- Co-authored-by: Derek Bailey <derekbailey@google.com>

* Start using pnpm * Add @npm * get more stuff set up * Get the analysis phase passing. * Get esbuild working? * Get it compiling? $ bazel build //tests/ts/... * Try to get the test working * test is passing * Get the other tests working * clarify comment * clean up a bit * Try to add another test * Add another test * clean up more * remove unused reference * Add e2e test * Get more of the test working * add lock file * Get test working on its own * Get e2e test passing * fix infinite recursion * Add comments * clean up some more * clean up more again * Source typescript version from package.json * run buildifier * lint * Fix unset `extra_env` * Incorporate feedback * run buildifier --------- Co-authored-by: Derek Bailey <derekbailey@google.com>

…google#7927) This reverts commit 4172c3f.

* Migrate from rules_nodejs to rules_js/rules_ts (take 2) This is the second version of patch google#7923. The first version got reverted because bazel query was failing: $ bazel --nosystem_rc --nohome_rc query tests(set('//...')) except tests(attr("tags", "manual", set('//...'))) ERROR: Traceback (most recent call last): File "/workdir/tests/ts/bazel_repository_test_dir/BUILD", line 6, column 22, in <toplevel> npm_link_all_packages(name = "node_modules") File "/var/lib/buildkite-agent/.cache/bazel/_bazel_buildkite-agent/ec321eb2cc2d0f8f91b676b6d4c66c29/external/npm/defs.bzl", line 188, column 13, in npm_link_all_packages fail(msg) Error in fail: The npm_link_all_packages() macro loaded from @npm//:defs.bzl and called in bazel package 'tests/ts/bazel_repository_test_dir' may only be called in bazel packages that correspond to the pnpm root package '' and pnpm workspace projects '' This was happening because the `.bazelrc` file only added `--deleted_packages` to the `build` command. We also need it for the `query` command. This second version of the patch fixes that. Original commit message: This patch migrates the current use of rules_nodejs to the new rules_js. rules_js is the intended replacement of rules_nodejs as per this note: https://github.com/aspect-build/rules_js#relationship-to-rules_nodejs > rules_js is an alternative to the build_bazel_rules_nodejs Bazel module > and accompanying npm packages hosted in > https://github.com/bazelbuild/rules_nodejs, which is now > unmaintained. All users are recommended to use rules_js instead. There are a few notable changes in this patch: 1. The `flatbuffer_ts_library` macro no longer accepts a `package_name` attribute. This is because rules_js appears to manage the import naming of dependencies via top-level `npm_link_package` targets. Users will have to migrate. 2. I added a few more arguments to `flatbuffer_library_public()`. These helped with exposing esbuild to `ts/compile_flat_file.sh`. 3. I pinned the version of `typescript` in `package.json` so that rules_ts can download the exact same version. rules_ts doesn't know what to do if the version isn't exact. 4. Since rules_js uses the pnpm locking mechanism, we now have a `pnpm-lock.yaml` file instead of a yarn lock file. 4. I added bazel targets for a few of the existing tests in `tests/ts`. They can be run with `bazel test //test/ts:all`. Since there is no flexbuffers bazel target, I did not add a bazel target for the corresponding test. 5. I added a separate workspace in `tests/ts/bazel_repository_test_dir/` to validate that the flatbuffers code can be imported as an external repository. You can run the test with `bazel test //test/ts:bazel_repository_test`. For this to work, I needed to expose a non-trivial chunk of the flatbuffers code to the test. I achieved this through some recursive `distribution` filegroups. This is inspired by rules_python's workspace tests. I did not do anything special to validate that the `gen_reflections` parameter works the same. This patch doesn't change anything about the TypeScript generation. As a side note: I am not an expert with rules_js. This patch is my attempt based on my limited understanding of the rule set. Fixes google#7817 * Fix the query --------- Co-authored-by: Derek Bailey <derekbailey@google.com>

* Start using pnpm * Add @npm * get more stuff set up * Get the analysis phase passing. * Get esbuild working? * Get it compiling? $ bazel build //tests/ts/... * Try to get the test working * test is passing * Get the other tests working * clarify comment * clean up a bit * Try to add another test * Add another test * clean up more * remove unused reference * Add e2e test * Get more of the test working * add lock file * Get test working on its own * Get e2e test passing * fix infinite recursion * Add comments * clean up some more * clean up more again * Source typescript version from package.json * run buildifier * lint * Fix unset `extra_env` * Incorporate feedback * run buildifier --------- Co-authored-by: Derek Bailey <derekbailey@google.com>

…google#7927) This reverts commit 4172c3f.

* Migrate from rules_nodejs to rules_js/rules_ts (take 2) This is the second version of patch google#7923. The first version got reverted because bazel query was failing: $ bazel --nosystem_rc --nohome_rc query tests(set('//...')) except tests(attr("tags", "manual", set('//...'))) ERROR: Traceback (most recent call last): File "/workdir/tests/ts/bazel_repository_test_dir/BUILD", line 6, column 22, in <toplevel> npm_link_all_packages(name = "node_modules") File "/var/lib/buildkite-agent/.cache/bazel/_bazel_buildkite-agent/ec321eb2cc2d0f8f91b676b6d4c66c29/external/npm/defs.bzl", line 188, column 13, in npm_link_all_packages fail(msg) Error in fail: The npm_link_all_packages() macro loaded from @npm//:defs.bzl and called in bazel package 'tests/ts/bazel_repository_test_dir' may only be called in bazel packages that correspond to the pnpm root package '' and pnpm workspace projects '' This was happening because the `.bazelrc` file only added `--deleted_packages` to the `build` command. We also need it for the `query` command. This second version of the patch fixes that. Original commit message: This patch migrates the current use of rules_nodejs to the new rules_js. rules_js is the intended replacement of rules_nodejs as per this note: https://github.com/aspect-build/rules_js#relationship-to-rules_nodejs > rules_js is an alternative to the build_bazel_rules_nodejs Bazel module > and accompanying npm packages hosted in > https://github.com/bazelbuild/rules_nodejs, which is now > unmaintained. All users are recommended to use rules_js instead. There are a few notable changes in this patch: 1. The `flatbuffer_ts_library` macro no longer accepts a `package_name` attribute. This is because rules_js appears to manage the import naming of dependencies via top-level `npm_link_package` targets. Users will have to migrate. 2. I added a few more arguments to `flatbuffer_library_public()`. These helped with exposing esbuild to `ts/compile_flat_file.sh`. 3. I pinned the version of `typescript` in `package.json` so that rules_ts can download the exact same version. rules_ts doesn't know what to do if the version isn't exact. 4. Since rules_js uses the pnpm locking mechanism, we now have a `pnpm-lock.yaml` file instead of a yarn lock file. 4. I added bazel targets for a few of the existing tests in `tests/ts`. They can be run with `bazel test //test/ts:all`. Since there is no flexbuffers bazel target, I did not add a bazel target for the corresponding test. 5. I added a separate workspace in `tests/ts/bazel_repository_test_dir/` to validate that the flatbuffers code can be imported as an external repository. You can run the test with `bazel test //test/ts:bazel_repository_test`. For this to work, I needed to expose a non-trivial chunk of the flatbuffers code to the test. I achieved this through some recursive `distribution` filegroups. This is inspired by rules_python's workspace tests. I did not do anything special to validate that the `gen_reflections` parameter works the same. This patch doesn't change anything about the TypeScript generation. As a side note: I am not an expert with rules_js. This patch is my attempt based on my limited understanding of the rule set. Fixes google#7817 * Fix the query --------- Co-authored-by: Derek Bailey <derekbailey@google.com>

philsc added 26 commits April 5, 2023 20:53

Start using pnpm

dd86410

Add @npm

0d86b93

get more stuff set up

9bd3543

Get the analysis phase passing.

f7e2b49

Get esbuild working?

e7898ba

Get it compiling?

7fbeeb5

$ bazel build //tests/ts/...

Try to get the test working

e7dc96a

test is passing

9a6a2b2

Get the other tests working

abef57e

clarify comment

3bfb26b

clean up a bit

125feec

Try to add another test

ae1e96c

Add another test

ce99fa7

clean up more

9ed5196

remove unused reference

2f59eff

Add e2e test

d22a897

Get more of the test working

1f90fdd

add lock file

f23ef3d

Get test working on its own

8e5dace

Get e2e test passing

631578d

Merge remote-tracking branch 'origin/master' into HEAD

5731c9b

fix infinite recursion

18770a6

Add comments

ed53297

clean up some more

9649f61

clean up more again

62cc288

Source typescript version from package.json

c1d2cfa

github-actions bot added codegen Involving generating code from schema grpc javascript json labels Apr 25, 2023

bjornharrtell previously approved these changes Apr 25, 2023

View reviewed changes

dbaileychess and others added 3 commits April 25, 2023 22:26

Merge branch 'master' into unreviewed/phil/port-to-rules-js

24f6e40

Merge remote-tracking branch 'origin/master' into HEAD

17a8f7c

Incorporate feedback

26c77c2

philsc dismissed bjornharrtell’s stale review via 26c77c2 April 27, 2023 04:14

philsc requested a review from jkuszmaul April 27, 2023 04:15

run buildifier

78186df

jkuszmaul reviewed Apr 27, 2023

View reviewed changes

dbaileychess approved these changes Apr 27, 2023

View reviewed changes

Merge branch 'master' into unreviewed/phil/port-to-rules-js

b082765

Merge branch 'master' into unreviewed/phil/port-to-rules-js

3e1ba75

dbaileychess enabled auto-merge (squash) April 28, 2023 18:14

dbaileychess merged commit 4172c3f into google:master Apr 28, 2023

dbaileychess added a commit that referenced this pull request Apr 28, 2023

Revert "Migrate from rules_nodejs to rules_js/rules_ts (#7923)"

d155575

This reverts commit 4172c3f.

dbaileychess mentioned this pull request Apr 28, 2023

Revert "Migrate from rules_nodejs to rules_js/rules_ts" #7927

Merged

dbaileychess added a commit that referenced this pull request Apr 28, 2023

Revert "Migrate from rules_nodejs to rules_js/rules_ts (#7923)" (#7927)

e7dc252

This reverts commit 4172c3f.

philsc mentioned this pull request Apr 29, 2023

Migrate from rules_nodejs to rules_js/rules_ts (take 2) #7928

Merged

jochenparm pushed a commit to jochenparm/flatbuffers that referenced this pull request Oct 29, 2024

Revert "Migrate from rules_nodejs to rules_js/rules_ts (google#7923)" (…

4a7447a

…google#7927) This reverts commit 4172c3f.

jochenparm pushed a commit to jochenparm/flatbuffers that referenced this pull request Oct 29, 2024

Revert "Migrate from rules_nodejs to rules_js/rules_ts (google#7923)" (…

16477d7

…google#7927) This reverts commit 4172c3f.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate from rules_nodejs to rules_js/rules_ts #7923

Migrate from rules_nodejs to rules_js/rules_ts #7923

philsc commented Apr 25, 2023 •

edited

Loading

jkuszmaul left a comment

dbaileychess commented Apr 28, 2023

philsc commented Apr 28, 2023

dbaileychess commented Apr 28, 2023

philsc commented Apr 29, 2023

philsc commented Apr 29, 2023

Migrate from rules_nodejs to rules_js/rules_ts #7923

Migrate from rules_nodejs to rules_js/rules_ts #7923

Conversation

philsc commented Apr 25, 2023 • edited Loading

jkuszmaul left a comment

Choose a reason for hiding this comment

dbaileychess commented Apr 28, 2023

philsc commented Apr 28, 2023

dbaileychess commented Apr 28, 2023

philsc commented Apr 29, 2023

philsc commented Apr 29, 2023

philsc commented Apr 25, 2023 •

edited

Loading