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

Allow vite to split dynamic imports to multiple files #744

Merged
merged 1 commit into from
Feb 14, 2023

Conversation

arbassett
Copy link
Contributor

@arbassett arbassett commented Feb 12, 2023

related #739

by not allowing vite to split dynamic imports rollup is unable to do any dead code elimination... with the current setup vite will always include dynamic imports even if they are never available like clientOnly in ssr. also vite's default output in esm so setting it to esm its redundant

How does this effect adapters?

  • For adapters that use file output nothing is changed it will still output a single file
  • For adapters that use dir output rollup will dump the extra files in the output folder which is just dist in all cases currently and from my testing the frameworks had no issue picking up the extra files.

to confirm that client only is removed i have a example repo here https://github.com/arbassett/solid-start-clientOnly-fix with the patch and committed dist dir to show that the clientOnly import was removed from the server build

@ryansolid
Copy link
Member

I really want to look back at why we added this. It feels like it was for a reason. But I'm glad you found the problem. I'm pretty sure routing removes dynamic imports on server anyway so it probably isn't that. It looks like the adapters pass so it isn't that either. There are some consequences for synchronous SSR will dynamic imports but it probably doesn't matter enough.

@arbassett
Copy link
Contributor Author

I'm not familiar with how solid routers internals work but from the bundler perspective having two files and one file doesn't change the end result being a Promise based import

without patch

const Lazy$2 = lazy(() => Promise.resolve().then(() => Lazy$1));

with Patch

const Lazy = lazy(() => import('./Lazy.b41f2dd3-bba4a033.js'));

hopefully this info helps your decision

@ryansolid
Copy link
Member

ryansolid commented Feb 13, 2023

With the router we just remove the lazy on the server. We didn't always. The other reason I could have seen was like quick solve for bundling for old Cloudflare stuff.. But like everything passes here so it is probably fine now. Some legacy thing long forgotten.

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.

None yet

2 participants