search

lightstep / lightstep-tracer-javascript

Lightstep distributed tracing library for Node.js and the browser
https://lightstep.com
MIT License
77 stars 66 forks source link

Split JavaScript bundle into thrift and proto versions (BREAKING CHANGE) #116

Closed frenchfrywpepper closed 5 years ago

frenchfrywpepper commented 6 years ago

This restores the -thrift.min.js version to very close to the size that it was before proto was introduced.

Users will need to modify existing references to lightstep-tracer.min.js to either lightstep-tracer-thrift.min.js or lightstep-tracer-proto.min.js.

If you would like to continue using Thrift you should also add the key/value transport=thrift as an argument to your lightstep.Tracer initialization.

resolves #113

frenchfrywpepper commented 6 years ago

@lawnsea I would love to get your feedback on this PR. It addresses the size of the thrift javascript, restoring it to the original 70ish KB.

nimeshksingh commented 5 years ago

Because this is a breaking change, this will likely be the first time many folks become aware of the proto option. As is the proto option actually does not work because by default proto uint64s are treated as javascript Numbers, but Numbers aren't big enough to handle large uint64s. We probably want to updated the proto and regenerate as indicated in the solution to this issue: https://github.com/protocolbuffers/protobuf/issues/3666

felixfbecker commented 5 years ago

It would be great if this worked not through global variables but through different entry points so self-bundling is possible (see also #138)

felixfbecker commented 5 years ago

I.e. thrift and proto libraries should be removed automatically by tree shaking depending on what is imported and used, not depending on what global variables are set.

lawnsea commented 5 years ago

@frenchfrywpepper I don't have any significant feedback on the PR other than 👏 that the bundle size is back to normal.