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

Automatically add Svelte component libraries to ssr.noExternal #904

Closed
Rich-Harris opened this issue Apr 6, 2021 · 17 comments · Fixed by #1148
Closed

Automatically add Svelte component libraries to ssr.noExternal #904

Rich-Harris opened this issue Apr 6, 2021 · 17 comments · Fixed by #1148
Milestone

Comments

@Rich-Harris
Copy link
Member

Is your feature request related to a problem? Please describe.
Vite will exclude many node_modules packages from the SSR bundle it generates, using a set of heuristics. While they're subject to change, those heuristics typically exclude Svelte component libraries from the bundle, and so we need to explicitly add them to ssr.noExternal, as in #806 or the kit.svelte.dev config.

Describe the solution you'd like
A convention exists for declaring a package to be a component library — pkg.svelte. It's not used universally (e.g. Svelte GL and site-kit don't use it, which I was slightly surprised about. not sure what i was thinking) but with #518 we can make it more of a standard.

We could forcibly add packages with a pkg.svelte to ssr.noExternal, in addition to any values the user may have specified. This would reduce the number of bugs along the lines of #806.

Relatedly, I can't remember the reason we added the current ssr.noExternal block to the template...

vite: {
ssr: {
noExternal: Object.keys(pkg.dependencies || {})
}
}
...but its presence feels like a design failure, and I think we should remove it and try and solve whatever issues arise a different way (by having adapters create their own single file bundles if appropriate, etc).

Describe alternatives you've considered
Solve it with documentation. We could also force devDependencies to always be bundled rather than dependencies, and expect people to understand the difference.

How important is this feature to you?
Not solving this one way or another is going to cause people to get mad at us.

@GrygrFlzr
Copy link
Member

the reason we added the current ssr.noExternal block

It's a workaround for getting Vite to bundle the dependencies into the application for serverless use. The targets that our current adapters use completely skip the platform's bundling step, so we have to do it ourselves. For example, our use of .vercel_build_output means that Vercel will never bundle the dependencies for us, similarly for adapter-netlify.

This absolutely feels like something the adapters should have control over especially because it's counterproductive for adapter-node, where bundling might not be a concern at all because you can just npm install in your environment, but probably needs the build step to be more tightly integrated with adapters, unless they can bundle post-build without significant drawbacks.

A really unfortunate related issue with this configuration is the difference between esbuild in dev and rollup in build. It's one of the biggest reasons a setup may work on dev but break on build (or vice versa).

@Rich-Harris
Copy link
Member Author

My thinking is that we could expose an API that essentially just runs esbuild on the Vite output, which should mean that external deps are treated identically in dev and prod?

@SillyFreak

This comment has been minimized.

@Shaun-Regenbaum

This comment has been minimized.

@benmccann
Copy link
Member

I filed a separate ticket to track the noExternal / bunding in build vs adapt issue: #1016

@benmccann
Copy link
Member

@GrygrFlzr metioned that some component libraries like carbon-component-svelte need to be added to noExternal or they complain about not being SSR-compatible, whereas svelte-materialify works perfectly out of the box as a dev dependency. Neither of us currently understand why this would be the case.

From my quick peek at it, both libraries provide both a main and module field so I would expect the Vite heuristic to behave the same in both cases:

@JBusillo
Copy link
Contributor

I have a couple of observations:

  1. All the dependencies of svelte-materialify are ESM. There is one dependency of carbon-components-svelte (clipboard-copy) that is a CJS module.
  2. The Vite dev server doesn't transform dependencies of any "root" dependencies. Since there is no tree-shaking in dev, it loads all dependencies (including clipboard-copy). For carbon-components-svelte, this generates the "module is not defined" error.

I cloned Vite, and made a change to run to transform all js modules to ESM:

// transformRequest.ts
import esbuild from 'esbuild'
...
if (file.endsWith("js")) {
    const r = await esbuild.transform(code, { format: "esm" })
    code = r.code
}
...

This transforms all .js files, even those that are already ESM, but esbuild doesn't modify the code in that case. Both carbon-components-svelte and svelte-materialify are in my package.json "dependencies".

Both 'npm run dev' and npm build/start appear to work.

Does this sound like a Vite bug, i.e., something they may have overlooked? I'm new at all this, and you may already know the source of this issue, so please excuse me if I'm not adding any value to this discussion.

@GrygrFlzr
Copy link
Member

It might be worth creating a PR to vite with your modifications. It should be a good starting point for improving compatibility with packages that use CJS somewhere in their dependency tree.

@JBusillo
Copy link
Contributor

I added the following issue: vitejs/vite#3024

An afterthought -- not sure what you meant by a PR. I'm very new to Open Source and Github infrastructures.

@benmccann
Copy link
Member

PR is a commonly used acronym for Pull Request. Thanks for the investigation on this!

@alexander-mart
Copy link
Contributor

alexander-mart commented Apr 19, 2021

@benmccann What exactly i should do to fix this error with carbon-components-svelte module?

<DataTable> is not a valid SSR component. You may need to review your build config to ensure that dependencies are compiled, rather than imported as pre-compiled modules

Reproduce:

  1. pnpm init svelte@next my-app and run
  2. add DataTable from example:
<script>
  import { DataTable } from "carbon-components-svelte";
</script>
  
<h1>Test</h1>

<DataTable
  sortable
  headers={[{ key: 'name', value: 'Name' }, { key: 'protocol', value: 'Protocol', sort: false }, { key: 'port', value: 'Port' }, { key: 'rule', value: 'Rule' }]}
  rows={[{ id: 'a', name: 'Load Balancer 3', protocol: 'HTTP', port: 3000, rule: 'Round robin' }, { id: 'b', name: 'Load Balancer 1', protocol: 'HTTP', port: 443, rule: 'Round robin' }, { id: 'c', name: 'Load Balancer 2', protocol: 'HTTP', port: 80, rule: 'DNS delegation' }, { id: 'd', name: 'Load Balancer 6', protocol: 'HTTP', port: 3000, rule: 'Round robin' }, { id: 'e', name: 'Load Balancer 4', protocol: 'HTTP', port: 443, rule: 'Round robin' }, { id: 'f', name: 'Load Balancer 5', protocol: 'HTTP', port: 80, rule: 'DNS delegation' }]}
/>

my svelte.config.json from template:

...
		vite: {
			ssr: {
				noExternal: Object.keys(pkg.dependencies || {})
			}
		}
...

Solutions from FAQ

  1. Try moving the package between dependencies and devDependencies — ✔️ don't helps
  2. Try to include or exclude it in optimizeDeps — where i can see an example?

Something like that?

...
		vite: {
			ssr: {
				noExternal: Object.keys(pkg.dependencies || {})
			},
			optimizeDeps: {
				include: [],
				exclude: ["carbon-components-svelte"],
			},
		}

@JBusillo
Copy link
Contributor

JBusillo commented Apr 20, 2021

@GreenRobot777 -- I created a project as you described -- placing carbon-components-svelte in 'dependencies'. I get the error:

[vite] Error when evaluating SSR module /node_modules/.pnpm/clipboard-copy@3.2.0/node_modules/clipboard-copy/index.js?v=3f798977:
ReferenceError: module is not defined

I'm not sure why you get your error message instead of mine.

Regardless, the module "clipboard-copy" is a cjs module, and will cause the error as I described. I had submitted an issue to Vite -- even though 'Datatable' may not use 'clipboard-copy', the dev server loads all dependencies of 'carbon-components-svelte'.

When I do an 'pnpm run build' and 'pnpm run start', it works properly -- due to tree shaking.

Here's my environment:

System:
    OS: Linux 5.8 Ubuntu 20.04.2 LTS (Focal Fossa)
    CPU: (6) x64 Intel(R) Core(TM) i5-8400 CPU @ 2.80GHz
    Memory: 15.12 GB / 31.04 GB
    Container: Yes
    Shell: 5.0.17 - /bin/bash
  Binaries:
    Node: 14.16.1 - /usr/bin/node
    Yarn: 1.22.10 - /usr/local/bin/yarn
    npm: 7.9.0 - /usr/local/bin/npm
  Browsers:
    Brave Browser: 90.1.23.71
    Chrome: 90.0.4430.72
    Firefox: 87.0
  npmPackages:
    @sveltejs/kit: next => 1.0.0-next.83 
    svelte: ^3.29.0 => 3.37.0 
    vite: ^2.1.0 => 2.2.1 

@kaladivo
Copy link

kaladivo commented Apr 20, 2021

@GreenRobot777 This issue was already discussed in carbon-components: see the issue

Rich-Harris pushed a commit that referenced this issue Apr 22, 2021
* force Vite to bundle component libraries in SSR - fixes #904

* get things working

* lockfile, again

* update docs

* Update documentation/faq/70-packages.md

Co-authored-by: Ben McCann <322311+benmccann@users.noreply.github.com>

* remove ssr config from svelte.config.cjs

* changeset

Co-authored-by: Ben McCann <322311+benmccann@users.noreply.github.com>
@benaltair
Copy link

@Rich-Harris I'm reading through this issue trying to understand what the resolution was. Basically is this still needed in the config or will it not be necessary?

vite: {
	ssr: {
		noExternal: Object.keys(pkg.dependencies || {})
	}
}

@babichjacob
Copy link
Member

@Rich-Harris I'm reading through this issue trying to understand what the resolution was. Basically is this still needed in the config or will it not be necessary?

vite: {
	ssr: {
		noExternal: Object.keys(pkg.dependencies || {})
	}
}

It's not necessary. Go ahead and remove it and (hopefully) nothing bad will happen.

@janosh
Copy link
Contributor

janosh commented May 6, 2021

@benaltair See https://kit.svelte.dev/faq#packages.

Old beta versions of the SvelteKit template included the configuration value noExternal: Object.keys(pkg.dependencies || {}) in svelte.config.js. First, please check if this line is present in your project and remove it if so. Removing this line fixes most issues and has since been removed from the template.

@benaltair
Copy link

Thanks @babichjacob and @janosh 🙏🏻

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 a pull request may close this issue.