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

cluster-ui: don't include protobufjs in bundle #82835

Merged

Conversation

sjbarag
Copy link
Contributor

@sjbarag sjbarag commented Jun 13, 2022

Prior to the removal of yarn workspaces [1], compiled versions of
cluster-ui included a single copy of protobufjs because of the node
module resolution algorithm [2]. Essentially, yarn workspaces ensured
that only a single copy of [email protected] existed at
pkg/ui/node_modules/protobufjs, and that no copy existed in
pkg/ui/workspaces/db-console/src/js/node_modules. When cluster-ui's
webpack build process attempted to add crdb-protobuf-client to the
bundle, it detected the initial line of the protobuf client
(import * as $protobuf from "protobufjs/minimal") and checked the
following directories for a protobufjs folder:

pkg/ui/workspaces/db-console/src/js/node_modules (no protobufjs/)
pkg/ui/workspaces/db-console/src/node_modules    (does not exist)
pkg/ui/workspaces/db-console/node_modules        (no protobufjs/)
pkg/ui/workspaces/node_modules                   (does not exist)
pkg/ui/node_modules                              (v6.8.6 found!)

Cluster-ui also imported "protobufjs/minimal", and so the same process
repeated:

pkg/ui/workspaces/cluster-ui/src/foo/node_modules (does not exist)
pkg/ui/workspaces/cluster-ui/src/node_modules     (does not exist)
pkg/ui/workspaces/cluster-ui/node_modules         (no protobufjs)
pkg/ui/workspaces/node_modules                    (does not exist)
pkg/ui/node_modules                               (v6.8.6 found!)

Since both cluster-ui and crdb-protobuf-client resolved
"protobufjs/minimal" to the same path, webpack considered those the same
dependency and only includes a single copy.

It's important at this point to understand why cluster-ui imports
protobufjs at all, since crdb-protobuf-client is intended to hide the
use of protobufs from its consumers. Protobufjs uses @protobufjs/inquire
to load the module "long" (which provides support for 64 bit
two's-complement integer values in JavaScript [3]) by using an
obfuscated, eval-based require call rather than a standard require()
call or import statement [4]. This approach is intended to be hidden
from bundlers like webpack, but it's unclear why the protobufjs authors
think that's desirable. Regardless of the reason, protobufjs's
util.Long property is null when bundled with webpack, because
webpack is unable to provide the "long" module where util.Long is
initialized [5]. cluster-ui monkey-patches that util.Long property
immediately [6] in a way that webpack can support. And because
crdb-protobuf-client and cluster-ui share a single copy of the
protobufjs module, crdb-protobuf-client's copy of util.Long is
impacted as well!

When yarn workspaces were removed [1], this path overlapping no longer
applied. pkg/ui/workspaces/db-console/src/js/node_modules suddenly did
contain a copy of [email protected], and so did
pkg/ui/workspaces/cluster-ui/node_modules! When bundling, webpack
included both copies (despite the fact that they're equivalent),
ensuring cluster-ui and crdb-protobuf-client get their respective
copies. Visually:

  src/protobufInit.ts
  └ protobufjs/minimal -> ./node_modules/protobufjs
                          ^^^^^^^^^^^^^^^^^^^^^^^^^
                          util.Long monkey-patched

  src/foo.ts
  └ @cockroachlabs/crdb-protobuf-client -> ../db-console/src/js
    └ protobufjs/minimal -> ../db-console/src/js/node_modules/protobufjs
                            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
                            util.Long not monkey-patched

db-console was still functional in this state because it included its
own copy of crdb-protobuf-client as a direct dependency (which
includes its own copy of protobufjs) and its own protobufInit.ts [7].
Visually, db-console imports resolved to:

  src/protobufInit.ts
  └ protobufjs/minimal -> ./src/js/node_modules/protobufjs
                          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
                          util.Long monkey-patched

  src/bar.ts
  └ @cockroachlabs/crdb-protobuf-client -> ./src/js
    └ protobufjs/minimal -> ./src/js/node_modules/protobufjs
                            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
                            util.Long monkey-patched already,
                            which fixes cluster-ui's
                            src/foo.ts import (see above).

The direct dependency on crdb-protobuf-client coincidentally uses the
same path as the transitive dependency through cluster-ui, so webpack is
able to de-dupe them and all copies of protobufjs are conveniently
fixed.

Since CockroachDB Cloud doesn't take a manual dependency on
crdb-protobuf-client (it relies on the copy from cluster-ui) and also
doesn't attempt to monkey-patch protobufjs, CockroachDB Cloud isn't able
to ensure that crdb-protobuf-client's copy of util.Long has a value.
Preventing cluster-ui from packaging its own copy of protobufjs means
that its runtime monkey-patching in src/protobufInit.ts always applies
to the copy of protobufjs imported by crdb-protobuf-client, and thus
prevents having an un-patched copy. Don't include a second copy of
protobufjs in cluster-ui bundle.

[1] 5b6c271 (bazel: upgrade to rules_nodejs 5.4.2, 2022-05-17)
[2] https://nodejs.org/docs/latest-v16.x/api/modules.html#loading-from-node_modules-folders
[3] https://github.com/dcodeIO/long.js/tree/088e44e5e3343ef967698565678384fa474b003b
[4] https://github.com/protobufjs/protobuf.js/blob/2cdbba32da9951c1ff14e55e65e4a9a9f24c70fd/lib/inquire/index.js#L10-L17
[5] https://github.com/protobufjs/protobuf.js/blob/2cdbba32da9951c1ff14e55e65e4a9a9f24c70fd/src/util/minimal.js#L176-L182
[6] https://github.com/cockroachdb/cockroach/blob/aac9c44d1284fa93877815f61527a76e110dbda5/pkg/ui/workspaces/cluster-ui/src/protobufInit.ts
[7] https://github.com/cockroachdb/cockroach/blob/d82ac30cc7b3cb5566e347ae1fecd2025c5e0623/pkg/ui/workspaces/db-console/src/protobufInit.ts

Release note: None

@sjbarag sjbarag requested review from maryliag and a team June 13, 2022 17:50
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Prior to the removal of yarn workspaces [1], compiled versions of
cluster-ui included a single copy of protobufjs because of the node
module resolution algorithm [2]. Essentially, yarn workspaces ensured
that only a single copy of [email protected] existed at
pkg/ui/node_modules/protobufjs, and that no copy existed in
pkg/ui/workspaces/db-console/src/js/node_modules. When cluster-ui's
webpack build process attempted to add crdb-protobuf-client to the
bundle, it detected the initial line of the protobuf client
(`import * as $protobuf from "protobufjs/minimal"`) and checked the
following directories for a protobufjs folder:

    pkg/ui/workspaces/db-console/src/js/node_modules (no protobufjs/)
    pkg/ui/workspaces/db-console/src/node_modules    (does not exist)
    pkg/ui/workspaces/db-console/node_modules        (no protobufjs/)
    pkg/ui/workspaces/node_modules                   (does not exist)
    pkg/ui/node_modules                              (v6.8.6 found!)

Cluster-ui also imported "protobufjs/minimal", and so the same process
repeated:

    pkg/ui/workspaces/cluster-ui/src/foo/node_modules (does not exist)
    pkg/ui/workspaces/cluster-ui/src/node_modules     (does not exist)
    pkg/ui/workspaces/cluster-ui/node_modules         (no protobufjs)
    pkg/ui/workspaces/node_modules                    (does not exist)
    pkg/ui/node_modules                               (v6.8.6 found!)

Since both cluster-ui and crdb-protobuf-client resolved
"protobufjs/minimal" to the same path, webpack considered those the same
dependency and only includes a single copy.

It's important at this point to understand _why_ cluster-ui imports
protobufjs at all, since crdb-protobuf-client is intended to hide the
use of protobufs from its consumers. Protobufjs uses @protobufjs/inquire
to load the module "long" (which provides support for 64 bit
two's-complement integer values in JavaScript [3]) by using an
obfuscated, `eval`-based require call rather than a standard `require()`
call or `import` statement [4]. This approach is intended to be hidden
from bundlers like webpack, but it's unclear why the protobufjs authors
think that's desirable. Regardless of the reason, protobufjs's
`util.Long` property is `null` when bundled with webpack, because
webpack is unable to provide the "long" module where `util.Long` is
initialized [5]. cluster-ui monkey-patches that util.Long property
immediately [6] in a way that webpack can support. And because
crdb-protobuf-client and cluster-ui share a single copy of the
`protobufjs` module, crdb-protobuf-client's copy of util.Long is
impacted as well!

When yarn workspaces were removed [1], this path overlapping no longer
applied. pkg/ui/workspaces/db-console/src/js/node_modules suddenly _did_
contain a copy of [email protected], and so did
pkg/ui/workspaces/cluster-ui/node_modules! When bundling, webpack
included both copies (despite the fact that they're equivalent),
ensuring cluster-ui and crdb-protobuf-client get their respective
copies. Visually:

```
  src/protobufInit.ts
  └ protobufjs/minimal -> ./node_modules/protobufjs
                          ^^^^^^^^^^^^^^^^^^^^^^^^^
                          util.Long monkey-patched

  src/foo.ts
  └ @cockroachlabs/crdb-protobuf-client -> ../db-console/src/js
    └ protobufjs/minimal -> ../db-console/src/js/node_modules/protobufjs
                            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
                            util.Long not monkey-patched
```

db-console was still functional in this state because it included its
_own_ copy of crdb-protobuf-client as a direct dependency (which
includes its own copy of protobufjs) and its own protobufInit.ts [7].
Visually, db-console imports resolved to:

```
  src/protobufInit.ts
  └ protobufjs/minimal -> ./src/js/node_modules/protobufjs
                          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
                          util.Long monkey-patched

  src/bar.ts
  └ @cockroachlabs/crdb-protobuf-client -> ./src/js
    └ protobufjs/minimal -> ./src/js/node_modules/protobufjs
                            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
                            util.Long monkey-patched already,
                            which fixes cluster-ui's
                            src/foo.ts import (see above).
```

The direct dependency on crdb-protobuf-client coincidentally uses the
same path as the transitive dependency through cluster-ui, so webpack is
able to de-dupe them and all copies of protobufjs are conveniently
fixed.

Since CockroachDB Cloud doesn't take a manual dependency on
crdb-protobuf-client (it relies on the copy from cluster-ui) and also
doesn't attempt to monkey-patch protobufjs, CockroachDB Cloud isn't able
to ensure that crdb-protobuf-client's copy of util.Long has a value.
Preventing cluster-ui from packaging its own copy of protobufjs means
that its runtime monkey-patching in src/protobufInit.ts always applies
to the copy of protobufjs imported by crdb-protobuf-client, and thus
prevents having an un-patched copy. Don't include a second copy of
protobufjs in cluster-ui bundle.

[1] 5b6c271 (bazel: upgrade to rules_nodejs 5.4.2, 2022-05-17)
[2] https://nodejs.org/docs/latest-v16.x/api/modules.html#loading-from-node_modules-folders
[3] https://github.com/dcodeIO/long.js/tree/088e44e5e3343ef967698565678384fa474b003b
[4] https://github.com/protobufjs/protobuf.js/blob/2cdbba32da9951c1ff14e55e65e4a9a9f24c70fd/lib/inquire/index.js#L10-L17
[5] https://github.com/protobufjs/protobuf.js/blob/2cdbba32da9951c1ff14e55e65e4a9a9f24c70fd/src/util/minimal.js#L176-L182
[6] https://github.com/cockroachdb/cockroach/blob/aac9c44d1284fa93877815f61527a76e110dbda5/pkg/ui/workspaces/cluster-ui/src/protobufInit.ts
[7] https://github.com/cockroachdb/cockroach/blob/d82ac30cc7b3cb5566e347ae1fecd2025c5e0623/pkg/ui/workspaces/db-console/src/protobufInit.ts

Release note: None
@sjbarag sjbarag force-pushed the dont-bundle-protobufjs-in-cluster-ui branch from e1cb21d to 8b2467c Compare June 13, 2022 17:51
Copy link
Contributor

@maryliag maryliag left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained

@sjbarag
Copy link
Contributor Author

sjbarag commented Jun 14, 2022

bors r=maryliag

@sjbarag
Copy link
Contributor Author

sjbarag commented Jun 14, 2022

bors r-

@craig
Copy link
Contributor

craig bot commented Jun 14, 2022

Canceled.

@sjbarag
Copy link
Contributor Author

sjbarag commented Jun 14, 2022

As it turns out, taking this change and adding protobufjs to the CockroachDB Cloud UI breaks the "SQL Activity" view for 21.1 and 21.2 clusters. Which makes sense — those now have an extra copy of protobufjs that isn't patched, just like the original description 😬

Pausing on this for now while I work things out in CockroachDB Cloud-land.

@sjbarag
Copy link
Contributor Author

sjbarag commented Jun 14, 2022

This is safe to deploy — we had the same issues with the 21.2 client as we did with 22.1, but that was solved with a re-publish in #82901.

@sjbarag
Copy link
Contributor Author

sjbarag commented Jun 14, 2022

bors r+

@craig
Copy link
Contributor

craig bot commented Jun 14, 2022

Build failed:

@sjbarag
Copy link
Contributor Author

sjbarag commented Jun 14, 2022

Flaky test, let's try once more before rebasing

bors r+

@craig
Copy link
Contributor

craig bot commented Jun 14, 2022

Build succeeded:

@craig craig bot merged commit ddcfc8a into cockroachdb:master Jun 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants