Skip to content

Conversation

@pcj
Copy link
Contributor

@pcj pcj commented Oct 6, 2025

Following #238, emitted closure js code is broken.

The issues I observe are as follows:

Missing function .writeZigzagVarint64BigInt:

The binary/encoder.js has multiple references to a function that simply does not exist:

      case 'bigint':
        this.encoder_.writeZigzagVarint64BigInt(/** @type {bigint} */(value));
        break;

This PR shims that with:

  /**
   * Encodes a BigInt into its wire-format, zigzag-encoded varint
   * representation and stores it in the buffer.
   * @param {bigint} value The BigInt to convert.
   */
  writeZigzagVarint64BigInt(value) {
    this.writeZigzagVarint64String(value.toString());
  }

Missing shim for jspb.ByteSource

The js_generator.cc file emits broken code that includes @param {jspb.ByteSource} bytes The bytes to deserialize, but jspb.ByteSource no longer exists. Other code external to this repo still relies on this type also. The generator has been updated to reference the correct type, and a new shim file binary/bytesource_alias.js has been added.

Broken import

The code in internal_public.js appears incorrect:

const {Map: JspbMap} = goog.requireType('jspb.Map');

This code does compile:

const JspbMap = goog.requireType('jspb.Map');

Explicit non-null cast needed

The closure-compiler rejects jspb.Message.clone_(jspb.asserts.assert(o)) but is OK with jspb.Message.clone_(/** @type {!Object} */(jspb.asserts.assert(o)))

Out-of-date bazel packaging code

The deleted targets like :dist_files reference seemingly non-existent files and fails with Error in glob: glob pattern 'google/protobuf/*.js' didn't match anything, but allow_empty is set to False (the default value of allow_empty can be set with --incompatible_disallow_empty_glob)..

I took the liberty of removing these, and added a convenience glob to access the necessary .js files that represents the pbjs library. I could take this out of the PR and just use a patch instead however.

@dibenede
Copy link
Contributor

dibenede commented Oct 7, 2025

Sorry about that. We have bigint codepaths internally that are exposed at the setter/getter level. We don't presently have that functionality in open source, so I tried to remove the references in the reader/writer code during the backport. The correct thing to do here would be to clean up any remaining references that I previously missed.

Adding an explicit cast should be OK.

We actively rely on those packing targets, especially dist_zip. Removing them will break the release process, so please restore them.

@dibenede dibenede self-requested a review October 7, 2025 00:19
BUILD.bazel Outdated

filegroup(
name = "dist_all",
name = "javascript",
Copy link
Contributor

Choose a reason for hiding this comment

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

i have no objection to keeping this file group, but we need to keep the dist targets. They are used after running gulp dist (aka npm run build): https://github.com/protocolbuffers/protobuf-javascript/blob/main/.github/workflows/build.yml#L73-L101

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll just use a patch I guess. This should really be re-thought though... The root BUILD file of the project is simply broken. Among other things the project can't be published to the bazel central registry in this state.

Copy link
Contributor

Choose a reason for hiding this comment

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

You're not wrong. The existing state is extremely fragile and a general pain. I'd much prefer to get everything into bazel, but I'm not presently well acquainted with the non-google bazel ecosystem.

Taking a step back for a moment: was your original motivation here to try to publish this project to BCR? Is there an FR you'd consider filing to make this project more usable to you as a closure user?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have several projects that rely on rules_closure, which in a previous state made it possible to use open-source versions of the closure-library + closure-templates + protobuf-javascript. rules_closure has been stripped down to pretty much only support j2cl. I have a fork of rules_closure that adds all that back in, and relies on protobuf-javascript (https://github.com/stackb/rules_closure/blob/main/MODULE.bazel#L39-L46).

If you have interest, I could prepare a PR that adds the BCR and github workflow boilerplate to publish to the BCR, which would make it easier to consume.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm interested and would accept that PR.

Separately, # NOTE: cannot upgrade past protobuf editions..., we permit edition 2023 as of 4.0.0. Conformance failures are listed in https://github.com/protocolbuffers/protobuf-javascript/blob/main/conformance/failing_tests.txt

@dibenede dibenede merged commit 5c70f21 into protocolbuffers:main Oct 8, 2025
4 checks passed
@pcj pcj deleted the pcj/closure-js-fixes branch October 8, 2025 22:12
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.

2 participants