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

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

Closed
wants to merge 5 commits into from

Conversation

frenchfrywpepper
Copy link
Contributor

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

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.
Copy link

@aliceadelef aliceadelef left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mostly just read through and made sure that when the would Thrift was said, that there was . ! like there should be. Those all were good, no typos. Can't say my review can catch much more than that. So probably should get other eyes on this too.

if ((typeof TRANSPORT_PROTO === 'undefined') || TRANSPORT_PROTO) {
proto = require('./generated_proto/collector_pb.js'); // eslint-disable-line global-require
}
if ((typeof TRANSPORT_PROTO === 'undefined') || !TRANSPORT_PROTO) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So when it is undefined, we want to set both proto and thrift? I'm confused by this

Also I think nit why is thrift croutonThrift but proto just proto?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it's only defined by webpack which generates the min.js for use in the browser. When using node both will be set (because in node there aren't the same size constraints.)

return new crouton_thrift.Auth({
access_token : this._accessToken,
});
if ((typeof TRANSPORT_PROTO === 'undefined') || !TRANSPORT_PROTO) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since this exact set of checks was done above, would it make sense to just check if croutonThrift was set?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm assuming there is a reason that this approach was taken, but it is very scary relaying on this !TRANSPORT_PROTO everywhere. Is there no way to use a TRANSPORT_THRIFT? (I'm guessing no) if that is the case, could we pull that into a function or something? Just so that kind of scary check only has to actually live, in one place?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These code statements are executed by webpack so you have to look at it like a pre-compile step not a runtime check. TRANSPORT_PROTO isn't a real thing during runtime and croutonThrift isn't available until runtime, but we want to eliminate the code from even existing in the minified js, so we have to use "pre-compile time" variables.

The resulting generated js looks like this...

if (true) {...} or if (false) {...}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh! huh ok yeah that makes sense.

@frenchfrywpepper
Copy link
Contributor Author

@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
Copy link
Contributor

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: protocolbuffers/protobuf#3666

@jmacd jmacd removed their request for review December 12, 2018 23:42
@felixfbecker
Copy link
Contributor

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
Copy link
Contributor

felixfbecker commented Dec 19, 2018

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
Copy link

lawnsea commented Mar 6, 2019

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

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

Successfully merging this pull request may close these issues.

Minified payload increased 6X by #111
6 participants