-
Notifications
You must be signed in to change notification settings - Fork 316
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
Decrease browser bundle size #681
base: main
Are you sure you want to change the base?
Conversation
Thank you for the investigation and this PR!
Good point! I am no expert at transpiling and bundling, which seems to be a whole science on its own. Nowadays we can raise our requirements on the ECMAScript support by the user's bundler and browsers. Although this has to happen in a major release to avoid breaking changes in a minor release. That being said, I have recently worked a bit on moving tus-js-client to TypeScript (#683), which was also a good opportunity to raise the compilation target.
This dependency was added to support tus-js-client inside React Native environment, where That being said, ~10KB for base64 encoding seems a bit too much and there are two possible approaches I can think of
Ideally, we would only need a polyfill for React Native, but I am not sure how that would be possible.
Similar story as before. An advantage of using those packages instead of using environment-specific APIs is that we get more consistent behavior, which is not influenced by subtle different between the implementations.
I don't know about Volta, but such a change should be in a separate PR and not part of this one. That being said, maybe it's worth considering switching back to NPM, which does not have different versions as Yarn appears to have. |
Ah, I didn't think of React Native. Hm... not sure, but it's possible that using a dynamic import could work: const encodeBase64 = navigator.product === 'ReactNative'
? (await import('js-base64')).encode
: btoa At least when bundled with webpack, this should create its own chunk. But not sure about vite/rollup. But yeah, that seems more like a hack that relies on runtime-detection of react native. Perhaps it would be better to create two separate builds: one for web and one for react native? |
I am hesitant with dynamic imports as I fear that they make the entire setup more complex and brittle. A separate build would also be possible, but I would prefer we first explore solutions to reduce the size of the base64 encoder, either by adjusting the import statement or pulling the code into tus-js-client directly. |
Yeah, you could use this implementation. But polyfilling |
Ah, I've never done React Native development, but
|
Either way, I would avoid publishing the bundled files to npm. (See e.g. this StackOverflow question). Not sure how the TypeScript conversion is going, but it's possible that using |
We currently include a bundled version alongside the original source code and a transpiled version. The latter two are referenced in the package.json, so the bundler can decide whatever variant it prefers using. The bundled version is not used in such cases. It is only included, so that people can download the bundle from NPM or use it via services like jsDelivr. Under such circumstances, I don't think it's harmful to include a bundled version in the package. The StackOverflow question you linked seems to refer to package which only include a bundled version, which is not the case for tus-js-client (and we intend to keep it that way). |
closes #679
This PR does four things:
forceAllTransforms
, which was added in Remove core-js and provide transpiled ES Module output #192 by @goto-bus-stop (as part of ESM support), although the PR description seems to imply thatforceAllTransforms
wasn't intended. (this makes the esm browser bundle go from from 97KB down to 47KB)new Url
(saves 16KB)package.json
for https://volta.sh, so that you can easily install the old yarn version used in this project (feel free to remove that commit if you think it doesn't belong).