Skip to content

Add to existing protobuf namespace instead of overwriting it #1460

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

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