Skip to content

Conversation

@taylorcode
Copy link
Collaborator

Addresses a bug related to loading multiple pbjs-generated JS files on one page.

When loading .js files generated by pbjs (static-module) if those .js files contain protos that belong to the same namespace, they will clobber each other at runtime.

Repro

a.proto

syntax = "proto3";
package my_pkg;

message MyMessageA {
    string a_field = 1;
}

b.proto

syntax = "proto3";
package my_pkg;

message MyMessageB {
    string b_field = 1;
}
pbjs -t static-module -w es6 -o a.js a.proto
pbjs -t static-module -w es6 -o b.js b.proto

After running these commands, a.js and b.js both contain:

export const my_pkg = $root.my_pkg = (() => {

So if b.js is loaded after a.js, it will erase the entries created by a.js, and vice versa.

Solution
This PR makes a change to code generation so the protobuf namespaces are extended at runtime:

export const my_pkg = $root.my_pkg = ((my_pkg) => {
   ...
})($root.my_pkg || {});

This changed solved the problem for us at dropbox.

@alexander-fenster
Copy link
Contributor

Hi @taylorcode, would it be possible to have a test that covers this code generation change?

@taylorcode
Copy link
Collaborator Author

@alexander-fenster I regenerated the fixtures with node scripts/gentests.js and added a test, but I'm now skeptical that this is the right approach.

While writing the test I discovered that this is not an issue when --root is configured to be unique for every file; this only occurs when default setting is used since it adds all new packages as properties to a $protobuf.roots["default"].

@alexander-fenster
Copy link
Contributor

@taylorcode Yes, using different roots is what we do in Google Cloud client libraries. It still could make sense not to overwrite root though. Let us think about it a little bit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants