Skip to content

Fix closure-js output #251

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Oct 8, 2025
Merged

Fix closure-js output #251

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
91b1fff
Fix closure-js output
pcj Oct 6, 2025
fd0e3c9
Revert BUILD file change, remove bigint handling in writer.js
pcj Oct 8, 2025
263ee70
remove writeZigzagVarint64BigInt
pcj Oct 8, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
95 changes: 32 additions & 63 deletions BUILD.bazel
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,6 @@
#
# See also code generation logic under generator/

load("@rules_pkg//:mappings.bzl", "pkg_attributes", "pkg_files", "strip_prefix")
load("@rules_pkg//:pkg.bzl", "pkg_tar", "pkg_zip")
load("//:protobuf_javascript_release.bzl", "package_naming")

config_setting(
name = "x64_x86_windows",
values = {"cpu": "x64_x86_windows"},
Expand All @@ -31,65 +27,38 @@ config_setting(
values = {"cpu": "darwin_x86_64"},
)

package_naming(
name = "protobuf_javascript_pkg_naming",
platform = select({
":k8": "linux-x86_64", # currently the only supported build type in Github Actions
":x64_x86_windows": "win32",
":x64_windows": "win64",
":darwin_arm64": "osx-aarch_64",
":darwin_x86_64": "osx-x86_64",
"//conditions:default": "" # continues with current behavior when no --cpu is specified allowing existing internal builds to function
})
)

pkg_files(
name = "plugin_files",
srcs = ["//generator:protoc-gen-js"],
attributes = pkg_attributes(mode = "0555"),
prefix = "bin/",
)

pkg_files(
name = "dist_files",
srcs = glob([
"google/protobuf/*.js",
"google/protobuf/compiler/*.js"
]) + [
"google-protobuf.js",
"package.json",
"README.md",
"LICENSE.md",
"LICENSE-asserts.md",
],
strip_prefix = strip_prefix.from_root(""),
)

pkg_tar(
name = "dist_tar",
srcs = [
":dist_files",
":plugin_files",
],
extension = "tar.gz",
package_file_name = "protobuf-javascript-{version}-{platform}.tar.gz",
package_variables = ":protobuf_javascript_pkg_naming",
)

pkg_zip(
name = "dist_zip",
srcs = [
":dist_files",
":plugin_files",
],
package_file_name = "protobuf-javascript-{version}-{platform}.zip",
package_variables = ":protobuf_javascript_pkg_naming",
)

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

@dibenede dibenede Oct 8, 2025

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

@pcj pcj Oct 8, 2025

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

@dibenede dibenede Oct 8, 2025

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

@pcj pcj Oct 8, 2025

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

@dibenede dibenede Oct 8, 2025

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

srcs = [
":dist_tar",
":dist_zip",
]
"asserts.js",
"binary/any_field_type.js",
"binary/arith.js",
"binary/binary_constants.js",
"binary/bytesource.js",
"binary/bytesource_alias.js",
"binary/decoder.js",
"binary/decoder_alias.js",
"binary/encoder.js",
"binary/encoder_alias.js",
"binary/errors.js",
"binary/internal_buffer.js",
"binary/reader.js",
"binary/reader_alias.js",
"binary/repeated_field_type.js",
"binary/scalar_field_type.js",
"binary/test_utils.js",
"binary/utf8.js",
"binary/utils.js",
"binary/writer.js",
"binary/writer_alias.js",
"bytestring.js",
"debug.js",
"internal_bytes.js",
"internal_options.js",
"internal_public.js",
"map.js",
"message.js",
"unsafe_bytestring.js",
],
visibility = ["//visibility:public"],
)
9 changes: 9 additions & 0 deletions binary/bytesource_alias.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
/**
* @fileoverview Legacy alias for the old namespace used by encoder.js
*/
goog.module('jspb.ByteSource');
goog.module.declareLegacyNamespace();

const { ByteSource } = goog.require('jspb.binary.bytesource');

exports = ByteSource;
9 changes: 9 additions & 0 deletions binary/encoder.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,6 +191,15 @@ class BinaryEncoder {
this.writeSplitVarint64(utils.getSplit64Low(), utils.getSplit64High());
}

/**
* 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());
}

/**
* Encodes a JavaScript decimal string into its wire-format, zigzag-encoded
* varint representation and stores it in the buffer. Integers not
Expand Down
1 change: 1 addition & 0 deletions binary/utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ goog.module.declareLegacyNamespace();
const BinaryConstants = goog.require('jspb.BinaryConstants');
const { assert } = goog.require('goog.asserts');
const { isBigIntAvailable } = goog.require('jspb.internal_options');
const { ByteSource } = goog.require('jspb.binary.bytesource');
const { ByteString } = goog.require('jspb.bytestring');
const { decodeStringToUint8Array } = goog.require('goog.crypt.base64');
const { unsafeUint8ArrayFromByteString } = goog.require('jspb.unsafe_bytestring');
Expand Down
3 changes: 2 additions & 1 deletion generator/js_generator.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1855,6 +1855,7 @@ void Generator::GenerateRequiresImpl(const GeneratorOptions& options,
required->insert("jspb.internal.public_for_gencode");
required->insert("jspb.BinaryReader");
required->insert("jspb.BinaryWriter");
required->insert("jspb.binary.bytesource");
}
if (require_extension) {
required->insert("jspb.ExtensionFieldBinaryInfo");
Expand Down Expand Up @@ -3016,7 +3017,7 @@ void Generator::GenerateClassDeserializeBinary(const GeneratorOptions& options,
printer->Print(
"/**\n"
" * Deserializes binary data (in protobuf wire format).\n"
" * @param {jspb.ByteSource} bytes The bytes to deserialize.\n"
" * @param {jspb.binary.bytesource.ByteSource} bytes The bytes to deserialize.\n"
" * @return {!$class$}\n"
" */\n"
"$class$.deserializeBinary = function(bytes) {\n"
Expand Down
2 changes: 1 addition & 1 deletion internal_public.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ goog.module.declareLegacyNamespace();
const asserts = goog.require('goog.asserts');
const { BinaryReader } = goog.require('jspb.binary.reader');
const { BinaryWriter } = goog.requireType('jspb.binary.writer');
const {Map: JspbMap} = goog.requireType('jspb.Map');
const JspbMap = goog.requireType('jspb.Map');

/**
* Write this Map field in wire format to a BinaryWriter, using the given
Expand Down
4 changes: 2 additions & 2 deletions message.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1876,7 +1876,7 @@ jspb.Message.clone_ = function(obj) {
// NOTE:redundant null check existing for NTI compatibility.
// see b/70515949
clonedArray[i] = (typeof o == 'object') ?
jspb.Message.clone_(jspb.asserts.assert(o)) :
jspb.Message.clone_(/** @type {!Object} */(jspb.asserts.assert(o))) :
o;
}
}
Expand All @@ -1892,7 +1892,7 @@ jspb.Message.clone_ = function(obj) {
// NOTE:redundant null check existing for NTI compatibility.
// see b/70515949
clone[key] = (typeof o == 'object') ?
jspb.Message.clone_(jspb.asserts.assert(o)) :
jspb.Message.clone_(/** @type {!Object} */(jspb.asserts.assert(o))) :
o;
}
}
Expand Down