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

adapter-cloudflare seems to undo all code splitting, and worse #2963

Open
tv42 opened this issue Dec 1, 2021 · 19 comments · May be fixed by naiba4/kit#1
Open

adapter-cloudflare seems to undo all code splitting, and worse #2963

tv42 opened this issue Dec 1, 2021 · 19 comments · May be fixed by naiba4/kit#1

Comments

@tv42
Copy link

tv42 commented Dec 1, 2021

Describe the bug

I'm using cloudflare-adapter to deploy to Cloudflare Pages. My _worker.js is growing to ridiculous sizes, perhaps most relevantly, it's larger than my app with all its chunks!

$ du -sh --apparent-size .svelte-kit/output/ .svelte-kit/cloudflare/
1.2M	.svelte-kit/output/
3.4M	.svelte-kit/cloudflare/

My biggest dependency is mapboxgl, and I see it minified in .svelte-kit/cloudflare/**/_app/chunks/vendor-*.js. But, I also see it embedded in .svelte-kit/cloudflare/_worker.js. Why is it both split and not split?

Reproduction

TBD

Logs

$ npm run build
[ everything here is under 44 KiB ] 
.svelte-kit/output/client/compare/_app/chunks/vendor-6f46024b.js                                        901.61 KiB / gzip: 246.75 KiB

(!) Some chunks are larger than 500 KiB after minification. Consider:
- Using dynamic import() to code-split the application
- Use build.rollupOptions.output.manualChunks to improve chunking: https://rollupjs.org/guide/en/#outputmanualchunks
- Adjust chunk size limit for this warning via build.chunkSizeWarningLimit.
vite v2.6.13 building SSR bundle for production...
✓ 77 modules transformed.
.svelte-kit/output/server/app.js   106.58 KiB

Run npm run preview to preview your production build locally.

> Using @sveltejs/adapter-cloudflare
  ✔ done

System Info

System:
    OS: Linux 5.14 NixOS 21.11 (Porcupine) 21.11 (Porcupine)
    CPU: (8) x64 11th Gen Intel(R) Core(TM) i7-1165G7 @ 2.80GHz
    Memory: 36.93 GB / 62.61 GB
    Container: Yes
    Shell: 5.1.8 - /nix/store/xdxv22yb7qnjfyf997r9fx804761z14c-bash-interactive-5.1-p8/bin/bash
  Binaries:
    Node: 16.13.0 - /nix/store/26lslfmvwyhspppbinznh111fyaxx62z-nodejs-16.13.0/bin/node
    npm: 8.1.0 - /nix/store/26lslfmvwyhspppbinznh111fyaxx62z-nodejs-16.13.0/bin/npm
  Browsers:
    Firefox: 94.0.2
  npmPackages:
    @sveltejs/adapter-cloudflare: ^1.0.0-next.2 => 1.0.0-next.2
    @sveltejs/kit: next => 1.0.0-next.191
    svelte: ^3.34.0 => 3.44.0

Severity

blocking an upgrade

Additional Information

This is severe because Cloudflare Workers _worker.js limit is only 1.5 MB, and importing mapboxgl without splitting uses a huge part of that.

@tv42
Copy link
Author

tv42 commented Dec 1, 2021

So, this seems to be because of the esbuild.build({bundle: true}) call in the adapter.

If bundling everything into the single file is the desired behavior, why are the js files from .svelte-kit/output/client/compare/_app/chunks/ etc also copied into .svelte-kit/cloudflare? Now the content is there twice.

I cannot pass splitting: true to esbuild because that requires outdir which cannot be set at the same time as outfile and the adapter forces outfile.

Next up, trying to use dynamic imports.

@tv42
Copy link
Author

tv42 commented Dec 2, 2021

Well, dynamic imports manage to split the dependency into its own .svelte-kit/output/client/**/_app/chunks/MyComponent-*.js, which is then promptly bundled back into _worker.js. :-(

@Rich-Harris
Copy link
Member

This is how workers, well, work. _worker.js has to be a single file, it can't import from other modules (at least currently). Since MapboxGL happens to be the sort of library that doesn't really do anything outside the browser, what we really need is some way to specify that it shouldn't be included in the bundle. Not 100% sure what that looks like — it might work to dynamically import it inside an onMount, but if push comes to shove you can always just include it as a <script> tag in your app.html pointing to static/js/mapbox-gl.js and reference it as a global.

My biggest dependency is mapboxgl, and I see it minified in .svelte-kit/cloudflare/**/_app/chunks/vendor-*.js. But, I also see it embedded in .svelte-kit/cloudflare/_worker.js. Why is it both split and not split?

Everything in _app is a client-side asset, and your client-side assets are code-split.

@tv42
Copy link
Author

tv42 commented Dec 2, 2021

My understanding is that Cloudflare Pages does not bundle all the static html, images, etc into _worker.js (I think I heard it uses KV, but haven't seen that confirmed). It would be neat for _worker.js to import from this store too, instead of bundling, for large chunks.

I was able to use a dynamic import with a wrapper component like this:

<script lang="ts">
	import { onMount, SvelteComponent } from 'svelte';

	let mapView: null | typeof SvelteComponent;

	onMount(async () => {
		const module = await import('$lib/map_view/MapView.svelte');
		mapView = module.default;
	});
</script>

<svelte:component this={mapView} {...$$props} />

However, it seems this gets tricky to generalize:

  • you can't pass the import path as a prop, because then Vite says it can't analyze it, and the import fails
  • you can't put the import in the caller as a <LazyComponent importer= {() => import('$lib/map_view/MapView.svelte')} /> or such, because that still causes the large dependency to be bundled.

@benmccann benmccann changed the title cloudflare-adapter seems to undo all code splitting, and worse adapter-cloudflare seems to undo all code splitting, and worse Dec 2, 2021
@tv42
Copy link
Author

tv42 commented Dec 2, 2021

For posterity, that seemed to run import() a every time a view was toggled between a list and a map, and I wasn't sure of the performance implications of that, so I added a cache:

<!-- Dynamic import MapView, and thus mapbox-gl, to keep Cloudflare Pages `_worker.js` size small. -->
<script context="module" lang="ts">
	import type { SvelteComponent } from 'svelte';

    // Cache the dynamic import, so it isn't re-run on every Map/List button toggle.
    let componentCache: typeof SvelteComponent | null = null;
</script>

<script lang="ts">
	import { onMount } from 'svelte';

    let mapView: typeof SvelteComponent | null = null;

	onMount(async () => {
        if (componentCache === null) {
			console.log('import');
			const module = await import('$lib/map_view/MapView.svelte');
			componentCache = module.default;
		}
        mapView = componentCache
	});
</script>

{#if mapView}
  <svelte:component this={mapView} {...$$props} />
{/if}

I wish this was easier to do without hardcoding the import in this wrapper (see earlier comment).

@syffs
Copy link

syffs commented Aug 3, 2022

on the same topic of _worker.js size, is there a reason why it is not minified ?

In my case, it divides by 2 the worker size (~1.2MB => 600KB).

EDIT: it appears that cloudflare does its own minification/gzipping when publishing, which explains why some big raw worker still end up being OK with the 1MB worker size requirement.

@major-mayer
Copy link

I also stumbled across this problem.
When you upload the pre-built folder using Cloudflare's wrangler CLI, you don't even get an error message why the deployment has failed.
I had to put everything inside a Gitlab**.com** (you can't use your private Git instance) repo and do the build using Cloudflare's own GUI process, and then the error message came up:

Error: Failed to publish your Function. Got error: Your Functions script is over the 1 MiB size limit (workers.api.error.script_too_large

It took me a while to even find the error for this.

For me, the reason for my huuuge 6.4 MB _worker.js file was mainly the import of BabylonJS.
In addition, as @syffs said, the whole file is unminified, which wastes even more storage.

As a workaround, I changed the import of my BabylonJS wrapper class from this:

 import BabylonViewer from "./BabylonViewer";

to this:

onMount(async () => {
		let BabylonViewer = await import("./BabylonViewer");
		if (bjsCanvas) {
			let viewer = new BabylonViewer.default(bjsCanvas);
		}

	});

But there really should be a better way to specify which dependencies should belong into the _worker.js bundle.

@MarArMar

This comment was marked as spam.

@eltigerchino eltigerchino added feature / enhancement New feature or request pkg:adapter-cloudflare-workers and removed feature / enhancement New feature or request labels Apr 5, 2024
@eltigerchino
Copy link
Member

eltigerchino commented Apr 5, 2024

Here with a quick update:

We can update the cloudflare-workers adapter to not bundle first but the cloudflare adapter will need to wait.

@eltigerchino
Copy link
Member

eltigerchino commented Apr 6, 2024

After trying to get a no bundle deployment working for a cloudflare workers project, I'm not seeing the benefits of it. It almost always increases the upload size / gzip size.

cc: @dario-piotrowicz what are your thoughts on this?

Relevant:

We should however make it more apparent in the documentation (if it's not already) that importing a library into a +page.svelte file that has SSR enabled will include it in the server bundle. It is always better to dynamically import client-only libraries in an onMount function or to turn off SSR for that page if that library can only be used in the browser anyway.

@eltigerchino eltigerchino modified the milestones: soon, non-urgent Apr 6, 2024
@dario-piotrowicz
Copy link
Contributor


It almost always increases the upload size / gzip size.

I can imagine that bundling can reduce the size of a svelteKit app by doing some code optimization and also deduplication and whatnot (wrangler uses esbuild under the hood, which in turn can optimize things), so it's not extremely surprising to me that the app's size can decrease with bundling.

But anyways, if there's a good amount of dynamic imports in the production build I would still strongly recommend to use no-bundling, as we've seen that that can significantly decrease response time (we do leverage that in our Next.js adapter) and runtime improvements in the module registry are in the work which would even increase the efficiency of this (cloudflare/workerd#1553).


I'm not seeing the benefits of it

What applications have you tried non-bundling with? I would assume that benefits would be very negligible for small hello-world applications but become much more significant as the complexity of the app and the number of dynamic imports increases.

(Again, in our Next.js adapter, relying on no-bundling and dynamic imports basically made shipping large applications possible, applications which otherwise would fail the Cloudflare startup time limit validation).


@eltigerchino these are my two cents on this 🙂

I would also try to figure out why you see an increase in the app's size and understand if that's negligible, how it scales and if that can be potentially addressed/improved on the SvelteKit's side

In any case I am always here (and on discord) to help any way I can 😄

@eltigerchino
Copy link
Member

Thanks @dario-piotrowicz . I didn't realise it has a big impact on the start up time. In that case, should it be an option with the default as bundled?

When I was doing some testing on this, I used a medium sized project on the free plan of cloudflare pages (https://brgh.church). Unbundled, the size would be 800KB+ and bundled would be 500KB+ uncompressed, both within the 1MB free plan limit when compressed.

My incorrect assumption was the increased function size would affect the start up time but maybe being unbundled has a bigger benefit?

@dario-piotrowicz
Copy link
Contributor

Thanks @dario-piotrowicz . I didn't realise it has a big impact on the start up time. In that case, should it be an option with the default as bundled?

mh... maybe, but ideally I think the best would be to just to with unbundled and try to reduce the app size as much as possible, to basically make it objectively better than the bundled version

(but if that's not possible/desired adding an option could be a viable alternative)

When I was doing some testing on this, I used a medium sized project on the free plan of cloudflare pages (https://brgh.church). Unbundled, the size would be 800KB+ and bundled would be 500KB+ uncompressed, both within the 1MB free plan limit when compressed.

mh... I see... yeah that doesn't seem a very minimal difference... but I think it would really be valuable to understand what those extra 300KB contain.... is it possible that those contain code that your build process fails tree-shake? (or even browser code?)

My incorrect assumption was the increased function size would affect the start up time but maybe being unbundled has a bigger benefit?

yes, unbundled should definitely have a smaller startup time, but again, it depends on how many lazy boundaries the code has and how much code those allow not to evaluate (and after cloudflare/workerd#1553 to parse and compile)
(i.e. the larger the amount of unnecessary code dynamic imports can help skipping the greater the time benefit unbundled will have)

@eltigerchino
Copy link
Member

eltigerchino commented Apr 7, 2024

mh... I see... yeah that doesn't seem a very minimal difference... but I think it would really be valuable to understand what those extra 300KB contain.... is it possible that those contain code that your build process fails tree-shake? (or even browser code?)

Now that you mention it, it's very likely I didn't configure esbuild correctly while I was trying it out and that could be one of the reasons why. I was fiddling around to try and add splitting: true to the esbuild build options.

Currently, the adapter uses esbuild with this worker file as the entry point, which imports the index.js file of the SSR bundle output by Vite.

const external = ['cloudflare:*', ...compatible_node_modules.map((id) => `node:${id}`)];
try {
const result = await esbuild.build({
platform: 'browser',
// https://github.com/cloudflare/workers-sdk/blob/a12b2786ce745f24475174bcec994ad691e65b0f/packages/wrangler/src/deployment-bundle/bundle.ts#L35-L36
conditions: ['workerd', 'worker', 'browser'],
sourcemap: 'linked',
target: 'es2022',
entryPoints: [`${tmp}/_worker.js`],
outfile: `${dest}/_worker.js`,
allowOverwrite: true,
format: 'esm',
bundle: true,
loader: {
'.wasm': 'copy'
},
external,
alias: Object.fromEntries(compatible_node_modules.map((id) => [id, `node:${id}`])),
logLevel: 'silent'
});

@eltigerchino
Copy link
Member

eltigerchino commented Apr 9, 2024

A related esbuild issue concerning unbundled support evanw/esbuild#708 (or maybe I’m thinking about this the wrong way? Do we even need esbuild if we’re not bundling code? Or maybe we just need to copy the server code over?)

@MarArMar

This comment was marked as spam.

@dummdidumm

This comment was marked as off-topic.

@MarArMar

This comment was marked as off-topic.

@dummdidumm

This comment was marked as off-topic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants