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

force Vite to bundle component libraries in SSR #1148

Merged
merged 8 commits into from
Apr 22, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/few-points-taste.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@sveltejs/kit': patch
---

Force Vite to bundle Svelte component libraries in SSR
5 changes: 5 additions & 0 deletions .changeset/nervous-steaks-add.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'create-svelte': patch
---

Remove obsolete vite.ssr config from template
7 changes: 4 additions & 3 deletions documentation/faq/70-packages.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,14 @@
question: How do I fix the error I'm getting trying to include a package?
---

Most of these issues come from Vite trying to deal with non-ESM libraries. You may find it helpful to search for your error message in [the Vite issue tracker](https://github.com/vitejs/vite/issues).
Most of these issues come from Vite trying to deal with non-ESM libraries. You may find it helpful to search for your error message in [the Vite issue tracker](https://github.com/vitejs/vite/issues).

There are a number of known Vite issues, which cause errors in the following circumstances:
- [packages which use `exports` instead of `module.exports`](https://github.com/vitejs/vite/issues/2579).

- [CommonJS packages](https://github.com/vitejs/vite/issues/2579).
- [ESM library that imports a CJS library](https://github.com/vitejs/vite/issues/3024)
- [some UMD libraries](https://github.com/vitejs/vite/issues/2679)

Vite 2 is a relatively new library and over time we expect it to become easier to use non-ESM libraries with Vite. However, you might also consider asking the library author to distribute an ESM version of their package or even converting the source for the package entirely to ESM. ESM is now the standard way to write JavaScript libraries and while there are a lot of legacy packages out there the ecosystem will become much easier to work with as more libraries convert to ESM.

The most common workarounds would be to try moving the package between `dependencies` and `devDependencies` or trying to `include` or `exclude` it in `optimizeDeps`. SvelteKit currently asks Vite to bundle all your `dependencies` for easier deployment to serverless environment. This means that by moving a dependency to `devDependencies` Vite is no longer asked to bundle it. This may sidestep issues Vite encounters in trying to bundle certain libraries. Avoiding Vite bundling especially works for `adapter-node` and `adapter-static` where the bundling isn't necessary since you're not running in a serverless environment. We are considering [better alternatives](https://github.com/sveltejs/kit/issues/1016) to make this setup easier. You should also add any Svelte components to `ssr.noExternal`. [We hope to do this automatically in the future](https://github.com/sveltejs/kit/issues/904).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add back this description except for the part after "You should also add...". It's pretty confusing about what goes where and why and I think the extra explanation as to what's going on is helpful

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's out of date (though this did remind me to remove the vite.ssr block from svelte.config.cjs, so thanks). We don't ask Vite to bundle dependencies for serverless deployment; it bundles the stuff it thinks need to be bundled (based on its heuristic, the heuristic implemented in this PR, and the user's noExternal config), and adapters bundle everything else if they deem it necessary.

Technically speaking it was never correct; SvelteKit isn't asking for special treatment of dependencies, the project template was. Now that the vite.ssr config is removed from the template, swapping stuff between dependencies and devDependencies will make no difference, and nor should it — that was always a weird hack we embraced out of necessity.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, you're right that it was a bit imprecise about SvelteKit vs template

I didn't realize vite.ssr could be removed from the templates. I thought that was still needed as a trick @GrygrFlzr had introduced for bundling for deployment to serverless environments. I didn't think it could be removed without updating the adapters as described in #1016. If I'm wrong, maybe that issue can be closed now

Anyway, I'm happy it's gone. I just hope we didn't make it hard to use the adapters

The most common workarounds would be to `include` or `exclude` the offending package in [`optimizeDeps`](https://vitejs.dev/config/#dep-optimization-options), or to add it to [`ssr.external` or `ssr.noExternal`](https://vitejs.dev/config/#ssr-options).
8 changes: 1 addition & 7 deletions packages/create-svelte/shared/+typescript/svelte.config.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -15,12 +15,6 @@ module.exports = {
adapter: node(),

// hydrate the <div id="svelte"> element in src/app.html
target: '#svelte',

vite: {
ssr: {
noExternal: Object.keys(pkg.dependencies || {})
}
}
target: '#svelte'
}
};
8 changes: 1 addition & 7 deletions packages/create-svelte/shared/-typescript/svelte.config.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -10,12 +10,6 @@ module.exports = {
adapter: node(),

// hydrate the <div id="svelte"> element in src/app.html
target: '#svelte',

vite: {
ssr: {
noExternal: Object.keys(pkg.dependencies || {})
}
}
target: '#svelte'
}
};
10 changes: 4 additions & 6 deletions packages/kit/src/core/build/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import fs from 'fs';
import path from 'path';
import { rimraf } from '../filesystem/index.js';
import create_manifest_data from '../../core/create_manifest_data/index.js';
import { copy_assets, posixify, resolve_entry } from '../utils.js';
import { copy_assets, get_no_external, posixify, resolve_entry } from '../utils.js';
import { create_app } from '../../core/create_app/index.js';
import vite from 'vite';
import svelte from '@sveltejs/vite-plugin-svelte';
Expand Down Expand Up @@ -438,11 +438,9 @@ async function build_server(
// @ts-ignore
ssr: {
...user_config.ssr,
noExternal: [
'svelte',
'@sveltejs/kit',
...((user_config.ssr && user_config.ssr.noExternal) || [])
]
// note to self: this _might_ need to be ['svelte', '@sveltejs/kit', ...get_no_external()]
// but I'm honestly not sure. roll with this for now and see if it's ok
noExternal: get_no_external(cwd, user_config.ssr && user_config.ssr.noExternal)
},
optimizeDeps: {
entries: []
Expand Down
6 changes: 5 additions & 1 deletion packages/kit/src/core/dev/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import { create_app } from '../../core/create_app/index.js';
import { rimraf } from '../filesystem/index.js';
import { ssr } from '../../runtime/server/index.js';
import { getRawBody } from '../http/index.js';
import { copy_assets } from '../utils.js';
import { copy_assets, get_no_external } from '../utils.js';
import svelte from '@sveltejs/vite-plugin-svelte';
import { get_server } from '../server/index.js';
import '../../install-fetch.js';
Expand Down Expand Up @@ -107,6 +107,10 @@ class Watcher extends EventEmitter {
optimizeDeps: {
...user_config.optimizeDeps,
entries: []
},
ssr: {
...user_config.ssr,
noExternal: get_no_external(this.cwd, user_config.ssr && user_config.ssr.noExternal)
}
});

Expand Down
32 changes: 32 additions & 0 deletions packages/kit/src/core/utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -74,3 +74,35 @@ export function resolve_entry(entry) {
export function posixify(str) {
return str.replace(/\\/g, '/');
}

/**
* Get a list of packages that use pkg.svelte, so they can be added
* to ssr.noExternal. This is done on a best-effort basis to reduce
* the frequency of 'Must use import to load ES Module' and similar
* @param {string} cwd
* @returns {string[]}
*/
function find_svelte_packages(cwd) {
const pkg_file = path.join(cwd, 'package.json');
if (!fs.existsSync(pkg_file)) return [];

const pkg = JSON.parse(fs.readFileSync(pkg_file, 'utf8'));

const deps = [...Object.keys(pkg.dependencies || {}), ...Object.keys(pkg.devDependencies || {})];

return deps.filter((dep) => {
const dep_pkg_file = path.join(cwd, 'node_modules', dep, 'package.json');
if (!fs.existsSync(dep_pkg_file)) return false;

const dep_pkg = JSON.parse(fs.readFileSync(dep_pkg_file, 'utf-8'));
return !!dep_pkg.svelte;
});
}

/**
* @param {string} cwd
* @param {string[]} [user_specified_deps]
*/
export function get_no_external(cwd, user_specified_deps = []) {
return [...user_specified_deps, ...find_svelte_packages(cwd)];
}
65 changes: 6 additions & 59 deletions pnpm-lock.yaml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.