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

Forwarding email communication #1390

Open
dcodeIO opened this issue Apr 16, 2020 · 12 comments
Open

Forwarding email communication #1390

dcodeIO opened this issue Apr 16, 2020 · 12 comments

Comments

@dcodeIO
Copy link
Member

dcodeIO commented Apr 16, 2020

I've received two mails about protobuf.js today, which is quite an uncommon thing to happen nowadays. Thought I forward these here so everyone, especially the maintainers who joined, is informed. Here is a summary:

  • One party is depending on feat: parsed options #1256 to land, and is asking if they can help to get it in.
  • Another party has made changes to code generation, in that it splits emitted files into one per service / message / enum to reduce overhead on the frontend. They are asking if there is interest in this, if they open sourced it?

Interestingly, I also had one email about bytebuffer.js:

  • Is there interest in adding aliases like getInt for readInt to make it work more like Java's ByteBuffer class?
@dcodeIO
Copy link
Member Author

dcodeIO commented Apr 16, 2020

On a more general note, a good path forward appears to be to merge #1356 soonish, and continue from there by applying the new release processes, versioning etc. that have been set up?

@alexander-fenster
Copy link
Contributor

@dcodeIO Looking at #1356.

@alexander-fenster
Copy link
Contributor

@dcodeIO #1356 is good to go! Then we can complete the release automation (Cc: @bcoe) and keep moving.

As for the list of things to work on, #1234 is also high in my personal priority list (since it will make it possible to use CLI tools in hermetic environments like bazel, and we do need it).

Thank you for forwarding the email request, I will do my best here :)

@dcodeIO
Copy link
Member Author

dcodeIO commented Apr 17, 2020

Merged that PR meanwhile, thanks for fixing! Shall we make a 6.9.0 release with the new process next?

@bcoe
Copy link
Contributor

bcoe commented Apr 17, 2020

Shall we make a 6.9.0 release with the new process next?

This sounds great to me, @alexander-fenster let me know if you need any help rolling out today?

@bcoe
Copy link
Contributor

bcoe commented Apr 20, 2020

@dcodeIO @alexander-fenster we rolled out the first release using the new process this morning 🎉

Shall we follow up with the folks emailing you?

@bcoe
Copy link
Contributor

bcoe commented Apr 20, 2020

catching up, sounds like we'll also need to work on reviewing and merging. #1256? perhaps we can let our new release bake for a bit, and then prioritize landing that fix?

@cherryland
Copy link

  • Another party has made changes to code generation, in that it splits emitted files into one per service / message / enum to reduce overhead on the frontend. They are asking if there is interest in this, if they open sourced it?

No. Strictly speaking, protobuf.js is a generator, and not a compiler.

Dead code elimination is a classical task for a compiler. And JavaScript is no exception to this rule. Closure Compiler e.g. parses JavaScript, analyzes it, removes dead code and rewrites and minimizes what's left. This is common best practice.

ok

@alexander-fenster
Copy link
Contributor

@cherryland If I'm reading the original request correctly, it's just about splitting the code, not to do any optimization or dead code removal. If we run pbjs today, we get a huge .js file, and I can imagine why it could make sense to split it by services, messages, or anything else.

(as an example, this generated js file from one of our libraries weighs 1.1MB, and it's not the biggest one I saw)

@cherryland
Copy link

@alexander-fenster You are right brother, on JavaScript Island 🏝️ the traditional approach to reduce file size is mere code splitting and nothing else. This was acceptable when Netscape was a thing and no other tools existed. I understand that.

The output from protobuf.js is meant to be consumed as a library. A developer makes the necessary references to the output from protobuf.js and defines the chunks (or code splits) at a higher level. A JavaScript compiler goes ahead and strips the unused parts based upon the dependency graph of the chunk. And as a developer, you always have the option to build a second proto file.

@taylorcode
Copy link
Collaborator

taylorcode commented Apr 22, 2020

@cherryland hi, my team at Dropbox added the no-bundle feature, and maintains a fork of protobufjs (https://github.com/dropbox/protobuf.js) for this and other reasons we have PRs open for.

I'm not sure if what @dcodeIO mentioned in bullet 2 is this feature or something else (sounds similar but might not actually be the same), but if it is I think you may have a misunderstanding. The no-bundle flag isn't about eliminating dead code, it's about eliminating duplicate code.

pbjs generates a single bundle of JS for all of the transitive .protos. Therefore if you run pbjs on proto A and then proto B, and both depend on C, then you'll end up with two copies of C's generated JS code -- a.js will contain it, as well as b.js. This is a problem if you need to load proto A and B on the same page for two reasons:

  • code bloat - you're loading C's code twice. If you need to load many protos and they share dependencies, you'll end up with an explosion of duplicate code.

  • orphaned object references - because of the way protobuf creates the roots data structure at runtime, every time C is loaded into memory it replaces the previous instance of C. Therefore you can potentially end up with references to different instances of C. Practically I'm not sure what issues this could cause but best to be avoided.

To work around this you could use pbjs to generate one bundle of JS bundle per page, but this isn't ideal for two reasons:

  • http caching - if two pages both need C's code, then ideally you have one copy of it that can be shared.
  • on-demand loading - if you only have one bundle then you can't load parts of it as needed.

You could also generate one bundle for all pages, but this does not scale.

The fact that pbjs bundles at all is a little weird. The protoc tool doesn't do this for any language, including JS. And even for web browsers this isn't ideal for the reasons stated. If bundling is needed, asset bundlers (e.g. rollup / webpack) should be used for this.

Example

my/protos/c.proto

package my_protos_c;

message C {
   string some_field = 1;
}

my/protos/a.proto

package my_protos_a;
import "my/protos/c.proto";

message A {
  my_proto_c.C nested_msg_c = 1;
}

my/protos/b.proto

package my_protos_b;
import "my/protos/c.proto";

message B {
   my_proto_c.C nested_msg_c = 1;
}

Strategy 1: run pbjs on each proto
pbjs --target static-module --out a.js a.proto
pbjs --target static-module --out b.js b.proto

import {my_protos_a} from 'my/protos/a';
import {my_protos_b} from 'my/protos/b';

page loads C's code twice

Strategy 2: generate one js bundle per page with pbjs
pbjs --target static-module --out page_1.js a.proto
pbjs --target static-module --out page_2.js b.proto

page 1

import {my_protos_a} from 'my/protos/page_1';

page 2

import {my_protos_b} from 'my/protos/page_2';

page 1 loads different copy of C than page 2

Strategy 3: generate one .js file for every .proto

pbjs --target static-module --path out/ --no-bundle a.proto
pbjs --target static-module --path out/ --no-bundle b.proto
pbjs --target static-module --path out/ --no-bundle c.proto

import {my_protos_a} from 'my/protos/a';
import {my_protos_b} from 'my/protos/b';

only 1 copy of C is loaded

page 1

import {my_protos_a} from 'my/protos/page_1';

page 2

import {my_protos_b} from 'my/protos/page_2';

page 1 loads same copy of C as page 2

@joshvarcheesy
Copy link

@dcodeIO Very large interest in the second bullet point. I just started to look into how to do this for my company.

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

No branches or pull requests

6 participants