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

packaging improvements #4978

Closed
pi0 opened this issue Nov 25, 2020 · 16 comments
Closed

packaging improvements #4978

pi0 opened this issue Nov 25, 2020 · 16 comments
Labels
kv-asset-handler Relating to @cloudflare/kv-asset-handler

Comments

@pi0
Copy link

pi0 commented Nov 25, 2020

Hi ✋ Whilist using kv-asset-handler I've seen some points of improvement worth to share. Would be happy to discuss on each, making sub-issues or going with some PRs to address.

Unnecessary files published inside npm package

Checking unpkg files like tsconfig.json or .github are published which increase install size. You may use files field in package.json to whitelist only dist (or also src)

Legacy dist

I can understand that cloudflare workers support (at least partial) of ES6 so we may keep things like await syntax which may help reduce final worker size (and probably more v8 optimization by using modern syntax). Using tsc, you may use ESNext but also careful not using unsupported features like optional chaining. Or using a modern tool like siroc (/cc @danielroe) or directly esbuild. (benefit of using siroc would be to also generate .mjs output that allows final compiler to do better tree-shaking, publishing public types.

Using full version of mime

mime has a lite entry (25 kB vs 9 kB) which is containing all of standard mimes. This can significantly reduce bundle size. You may import from mime/lite

@SukkaW
Copy link
Contributor

SukkaW commented Nov 25, 2020

so we may keep things like await syntax which may help reduce final worker size (and probably more v8 optimization by using modern syntax)

Yes, I do agree on the polyfill for async/await is not necessary since Cloudflare Workers already provides the support for it. But IMHO I prefer kv-asset-handler to be written in traditional Promise instead of async/await: although v8 improves the performance of async/await quite a lot, it is still slower than traditional Promise.

mime has a lite entry (25 kB vs 9 kB) which is containing all of standard mimes. This can significantly reduce bundle size. You may import from mime/lite

Although full mime is a bit large, the bundle size will not affect so much since Cloudflare has the 1 MiB size limit for the script.

@pi0
Copy link
Author

pi0 commented Nov 25, 2020

Yes, I do agree on the polyfill for async/await is not necessary since Cloudflare Workers already provides the support for it. But IMHO I prefer kv-asset-handler to be written in traditional Promise instead of async/await: although v8 improves the performance of async/await quite a lot, it is still slower than traditional Promise.

For sure. And since currently there are two await calls, splittting branches with two helper functions shouldn't be hard... Still there are more polyfills in current dist than async/await that not using transpilation could make it more faster and easier to tree-shake.

Although full mime is a bit large, the bundle size will not affect so much since Cloudflare has the 1 MiB size limit for the script.

That's right! But we are using it right now in a server-side-rendering solution. Size impact of each dependency matters when using kv-asset-handler. (it is currently fixed by an alias at bundler level)

(i might be wrong at this but a larger DB may also cause slighlty longer for mime lookup)

@SukkaW
Copy link
Contributor

SukkaW commented Nov 25, 2020

That's right! But we are using it right now in a server-side-rendering solution. Size impact of each dependency matters when using kv-asset-handler. (it is currently fixed by an alias at bundler level)

Since it is possible to serve "uncommon" files through Cloudflare Workers, at least we should make mime lite/full configurable. But then it becomes unsure whether dynamic import could be tree-shaken correctly.

@pi0
Copy link
Author

pi0 commented Nov 25, 2020

At least we should make mime lite/full configurable

Agreed. It might be somehow handled similar at wrangler level for aliasing to lite/full

Just to clarify this is what mentioned about lite:

There is also a "lite" version of this module that omits vendor-specific (*/vnd.*) and experimental (*/x-*) types. It weighs in at ~2.5KB, compared to 8KB for the full version

(sizes are in gzip)

@xtuc
Copy link
Member

xtuc commented Nov 26, 2020

I don't think performance is a consider when using async/await vs Promise. It won't have any noticeable impact in your worker. I would privilege the one that's easier to write/maintain.

On the other hand size is a concern, webpack should be able to swap out mime full for the lite version at build time. I'm concerned that using the lite version by default would break existing users.

@pi0
Copy link
Author

pi0 commented Nov 26, 2020

Fully agree with @xtuc points.

  • Using async/await makes code readable and letting v8 engine to optimize without tackling it
  • Directly using mime here is probably safest. We may implement alias with wrangler and add as a tip to documentation as a best practice if do not want to serve files with vendor/exp mime

@philipatkinson
Copy link
Contributor

Agree with much of this, as we are using an adapted version of kv-asset-handler in this context. We are part of the Durable Objects beta, so we've been reworking some of our dependencies to ES Modules where it will clean up the build by removing bundler boilerplate. For kv-asset-handler, this required a tweak of tsconfig.json and a quick rewrite/cleanup of the relevant bits of mime into TypeScript (in the context of how kv-asset-handler uses mime, it's just a wrapper around mime-db and the maintainer appears to prefer compatibility over modern JS structures).

If you want, I can formalise our branch with with a few tests, create a secondary build path for an ES module output and make the new 'mime wrapper' easy to instantiate with a different/subset of the mime-db (as per the lite version of mime) and submit a pull request?

@Cherry
Copy link
Contributor

Cherry commented May 19, 2021

Great discussion here, thanks all. I've got a few changes in cloudflare/kv-asset-handler#179 that help here, though I'd encourage any feedback and suggestions people have. That PR sets a files config for NPM publishing, to reduce the unnecessary publishing of configs, etc. as well as tweaks the TypeScript configuration just a little bit, to reduce the unnecessary polyfilling of generators (for async/await).

There's probably a lot more we could do with our build chain and/or TypeScript to further reduce the output size. With the recent changes I made, the only remaining polyfill really seems to be for async/await, which we could also likely remove in the future with proper ESM support.

@Cherry
Copy link
Contributor

Cherry commented Oct 15, 2021

cloudflare/kv-asset-handler#254 increased the TypeScript build target to ES2017 which removed the final async/await polyfill.

I believe the only thing left in this issue now is any further discussion around mime/mime/lite.

@MarvinMiles
Copy link

Size of my bundle increased from 5kb to 17kb after installing this package.
Execution time increased by ~10ms as well. (basing on Time to First Byte (TTFB) stat)

Not terrible but not cool either.

Screen.Recording.2022-02-17.at.9.35.27.PM.mov

@philipatkinson
Copy link
Contributor

I'm currently finalising changes on my fork (based around ES modules) which I will submit pull requests from soon. Certainly the mime stuff has a load of uncommon mime types which are unnecessary for the average user and could be significantly reduced in size. I would also guess a high percentage of people use wrangler rather than dynamically populating the asset manifest, in which case it should be possible to narrow down the mime list to the specific set required per user (although I can't be certain how much impact that would have on TTFB etc).

@danielroe
Copy link

For example, we narrow down our mimes and other types at build-time: https://github.com/nuxt/framework/blob/main/packages/nitro/src/rollup/plugins/static.ts#L10-L28

So any removal of these from the runtime package would be very nice 😁

@philipatkinson
Copy link
Contributor

That seems like a sensible approach - only at publish/build is there a chance to narrow the types to the specific subset needed.

Can't comment on what direction the CF people might want to go, but it would seem that this is probably a situation where the user has to take some responsibility for narrowing down the types - would guess that there are quite a few edge cases which would be difficult to cover if this package was overly simplified.

Will give some thought as to whether there is an easy way of having a user supplied mime list which, if using something like rollup, would remove the default list and thereby reduce the package size.

@GregBrimble GregBrimble transferred this issue from cloudflare/kv-asset-handler Feb 9, 2024
@GregBrimble GregBrimble added the kv-asset-handler Relating to @cloudflare/kv-asset-handler label Feb 9, 2024
@SukkaW
Copy link
Contributor

SukkaW commented Feb 10, 2024

Three years have passed since the issue was first raised, and yet the mime package remains the main factor contributing to the large bundle size.

Even when with mime/lite, the size of the bundled/minified package is still substantial at 12.1 KiB.

A noteworthy alternative is the mrmime package (whose maintainer is a former Cloudflare Staff—kudos to him!) that builds upon the mime/lite. After bundling and modification, the size of the mrmime can be reduced to 10.8 KiB, which is still a lot.

The data sources for both the mime and the mrmime packages are quite outdated and no longer actively maintained. The last update to the mime-db was a year ago, and [numerous issues remain unaddressed](https://github.com/jshttp/mime-db/issues.

Moreover, a larger worker dist size leads to slower bootstrap times, higher Time To First Byte (TTFB), and overall poorer performance (as mentioned previously where their workers' TTFB has increased by 10ms).

@philipatkinson
Copy link
Contributor

I may be wrong, but I am not sure there is a great appetite for a rework at CF on this one. I did pretty much the entire refactor you are describing in the PR I submitted above (replicating the functionality of mime/mime-lite to remove the dependency and make it more maintainable, ES modules etc) but it ends up being quite a substantial rewrite for questionable(?) gain.

In a sense, from our (customer) perspective, Pages has somewhat superseded kv-asset-handler (although, based on the defaults for Pages, I strongly suspect it is running kv-asset-handler or a modified version/derivative of it).

If Greg (or anyone else at CF) wants to revisit (some, or all of) the changes in my PR, or generally discuss, I'm happy to contribute as needed.

@penalosa
Copy link
Contributor

I’m going to close this issue for now. Going forward, we’ll revisit the assets system as part of Pages/Workers convergence, and will inherit the benefits of the Pages assets system.

In the meantime, Pages is the best choice for deploying sites with static assets.

Because of that, we're unlikely to refactor kv-asset-handler in any significant way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kv-asset-handler Relating to @cloudflare/kv-asset-handler
Projects
Status: Done
Development

No branches or pull requests

9 participants