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

Switch to Microbundle to reduce library size by 80% #359

Closed
wants to merge 1 commit into from

Conversation

developit
Copy link

Hiya! I noticed the size of this library could be reduced considerably while doing other optimization work, and figured I'd make the suggestion since I'd done the work anyway.

This replaces tsc -d with microbundle, reducing total size of the JS files in dist/ from 60kb to 12kb. I also added in an ESM build and an export map.

@jasonkuhrt
Copy link
Member

jasonkuhrt commented Jun 9, 2022

Thanks @developit (Same first name and we both live in Canada :D). I'm not convinced that libraries are the right place to do bundle optimization. Why not do this at the application layer?

@jasonkuhrt jasonkuhrt marked this pull request as ready for review June 9, 2022 03:06
@jasonkuhrt jasonkuhrt marked this pull request as draft June 9, 2022 03:06
@developit
Copy link
Author

@jasonkuhrt most of the optimization still needs to be done at the app level, however many of the most valuable optimizations (dead code elimination, cross-module inlining, module splitting, etc) are severely impacted by, or even disabled be, packages publishing CommonJS or UMD modules.

In addition to that, bundlers and optimizers can't undo suboptimal compiled output. As an example, this package includes a number of modules, and these are transpiled to ES5 before being published to npm. Each module contains polyfills and shims that are generated to support the source code's use of things like async/await and object spread. See the first 80 lines of this file:
https://unpkg.com/browse/graphql-request@4.3.0/dist/index.js

Because the transpilation process is done per-file and not by a bundler that is aware of the module graph, those helpers are duplicated in any file that uses those syntax features - see the first 48 lines in this other file:
https://unpkg.com/browse/graphql-request@4.3.0/dist/graphql-ws.js

Finally, I should point out that this PR doesn't change the structure of the generated modules - it's not bundling anything, and minification could be turned off if deemed unnecessary. (This is where the name "microbundle" is perhaps misleading, haha)

The important thing here is to use a tool that produces ES Modules in addition to CommonJS. Ideally, a tool should also generate modern (ESnext) versions of the modules - useful for apps not targeting older browsers, but also for app build systems that can then control how this module's modern code is transpiled (to produce fewer duplicate polyfills and more consistent syntax that compresses better).

For a bit of context - I'd originally opened this issue because this module was 10% of the JS weight of an application I'm working on (even after bundle optimization).

Hope that helps illuminate things!

@jasonkuhrt
Copy link
Member

jasonkuhrt commented Jun 10, 2022

Thanks @developit that starts to make sense for me. Some initial thoughts I have are:

  1. Can we leverage TS 4.7 to natively emit an ESM build to support applications building optimal bundles.
  2. Can we improve our TS config settings, e.g. make sure we're depending on tslib, make sure we target something modern including native Async Await, etc.

I'm all for bringing in more tooling when it is needed, but I suspect we haven't hit the ceiling yet on what is possible now with TS.

@developit
Copy link
Author

developit commented Jun 10, 2022

TS can emit ESM, no need to change versions to get that. Just set the {"module":"ESNext"} in the "compilerOptions" section of tsconfig.json.

To improve the output quality, you could set the "target" property to something like "es2017".

That being said, there are still a lot of applications that don't transpile things found in node_modules. Changing the projects tsconfig would be a big improvement, but would mean lots of folks wouldn't be able to upgrade and get those improvements. This is probably the main reason projects involve a tool like Rollup (or microbundle or tsdx, which are both just wrappers around Rollup): to generate multiple formats from the same source code. It's important for ESM and CJS, but also "ESM with modern syntax". It's a pain to do with tsconfig.

@jasonkuhrt
Copy link
Member

jasonkuhrt commented Jun 10, 2022

I haven't dug deep yet but I was under the impression that TS 4.7 can emit proper ESM now such as imports that include file extensions, etc. More here: https://devblogs.microsoft.com/typescript/announcing-typescript-4-7/#ecmascript-module-support-in-node-js. But maybe I'm missing your point.

It's a pain to do with tsconfig.

This might be where we differ, I don't actually find maintaining multiple tsconfigs too bad. I do a bit of that in my lib template repo for example https://github.com/jasonkuhrt/template-typescript-lib (but I haven't updated it to use 4.7 and emit proper ESM yet).

@developit
Copy link
Author

Ah - no, you're exactly right, I'd forgotten about the file extensions issue.

Interesting approach with the multiple tsconfigs - that actually doesn't seem bad at all. Not sure why I didn't think of running tsc multiple times.

The approach in that repo looks good to me FWIW. Think it'd be hard to airlift into this module?

@jasonkuhrt
Copy link
Member

jasonkuhrt commented Jun 10, 2022

Yeah I think we can bring in the changes without too much effort. Do you have an ESM project you are using this on? Even if not, would you be up to Guinea pig canary releases to make sure we get this done optimally?

@jasonkuhrt jasonkuhrt mentioned this pull request Jun 15, 2022
@gonzaloriestra
Copy link

Hi @jasonkuhrt! We are using this library in the Shopify CLI and would like it to make it an ESM dependency to improve performance (so that it can be loaded concurrently). So we are happy to help testing a canary release if needed. Thanks!

@jasonkuhrt jasonkuhrt deleted the branch graffle-js:master January 26, 2023 02:00
@jasonkuhrt jasonkuhrt closed this Jan 26, 2023
@jasonkuhrt
Copy link
Member

jasonkuhrt commented Feb 18, 2023

Hey @gonzaloriestra! You can try out the next pre-release that will include #455.

@gonzaloriestra
Copy link

@jasonkuhrt thanks a lot! We'll give it a try

@gonzaloriestra
Copy link

We have been testing the pre-release and it looks good, but we'll better wait for the next stable release to include it. Any plans for it? Thanks again.

@jasonkuhrt
Copy link
Member

In March for sure.

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.

3 participants