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

Conditional configuration in vite.config.js breaks prod build #5372

Closed
Conduitry opened this issue Jul 5, 2022 · 5 comments · Fixed by #5376
Closed

Conditional configuration in vite.config.js breaks prod build #5372

Conduitry opened this issue Jul 5, 2022 · 5 comments · Fixed by #5376
Labels
bug Something isn't working

Comments

@Conduitry
Copy link
Member

Conduitry commented Jul 5, 2022

Describe the bug

If your vite.config.js uses conditional configuration, Svelte stops trying to compile components files to JS during SSR, and the resulting build fails because Rollup/Acorn is trying to parse Svelte files as JS.

Reproduction

https://stackblitz.com/edit/sveltejs-kit-template-default-4wrfgp

Logs

❯ npm run build
$ vite build
vite v2.9.13 building for production...
✓ 34 modules transformed.
.svelte-kit/output/client/_app/immutable/assets/svelte-logo-87df40b8.svg                           1.85 KiB
.svelte-kit/output/client/_app/immutable/assets/fira-mono-cyrillic-ext-400-normal-3df7909e.woff2   15.40 KiB
.svelte-kit/output/client/_app/immutable/assets/fira-mono-cyrillic-400-normal-c7d433fd.woff2       8.89 KiB
.svelte-kit/output/client/_app/immutable/assets/fira-mono-greek-ext-400-normal-9e2fe623.woff2      7.33 KiB
.svelte-kit/output/client/_app/immutable/assets/fira-mono-greek-400-normal-a8be01ce.woff2          10.27 KiB
.svelte-kit/output/client/_app/immutable/assets/fira-mono-latin-ext-400-normal-6bfabd30.woff2      11.10 KiB
.svelte-kit/output/client/_app/immutable/assets/fira-mono-latin-400-normal-e43b3538.woff2          15.90 KiB
.svelte-kit/output/client/_app/immutable/assets/fira-mono-all-400-normal-1e3b098c.woff             75.55 KiB
.svelte-kit/output/client/_app/immutable/manifest.json                                             2.67 KiB
.svelte-kit/output/client/_app/immutable/start-ab798b44.js                                         23.25 KiB / gzip: 8.72 KiB
.svelte-kit/output/client/_app/immutable/pages/__layout.svelte-f0707ee1.js                         4.77 KiB / gzip: 1.86 KiB
.svelte-kit/output/client/_app/immutable/error.svelte-c8df2b3e.js                                  1.56 KiB / gzip: 0.75 KiB
.svelte-kit/output/client/_app/immutable/pages/about.svelte-692f0ea0.js                            2.51 KiB / gzip: 1.15 KiB
.svelte-kit/output/client/_app/immutable/pages/index.svelte-471ba9cf.js                            5.47 KiB / gzip: 2.42 KiB
.svelte-kit/output/client/_app/immutable/pages/todos/index.svelte-dda24c9b.js                      6.86 KiB / gzip: 2.89 KiB
.svelte-kit/output/client/_app/immutable/chunks/index-9fe72fd8.js                                  11.81 KiB / gzip: 5.02 KiB
.svelte-kit/output/client/_app/immutable/chunks/index-92d65ea7.js                                  0.43 KiB / gzip: 0.30 KiB
.svelte-kit/output/client/_app/immutable/chunks/singletons-d1fb5791.js                             0.05 KiB / gzip: 0.07 KiB
.svelte-kit/output/client/_app/immutable/assets/pages/__layout.svelte-da7c5ce3.css                 5.08 KiB / gzip: 1.56 KiB
.svelte-kit/output/client/_app/immutable/assets/pages/about.svelte-bf4528fa.css                    0.11 KiB / gzip: 0.10 KiB
.svelte-kit/output/client/_app/immutable/assets/pages/index.svelte-e12bfb09.css                    1.45 KiB / gzip: 0.52 KiB
.svelte-kit/output/client/_app/immutable/assets/pages/todos/index.svelte-a910a02d.css              3.70 KiB / gzip: 1.03 KiB
vite v2.9.13 building SSR bundle for production...
✓ 0 modules transformed.
transforming (1) .svelte-kit/build/index.js[rollup-plugin-dynamic-import-variables] Unexpected token (1:0)
file: /home/projects/sveltejs-kit-template-default-4wrfgp/src/routes/__layout.svelte:1:0
[vite-plugin-svelte-kit] Unexpected token (1:0)
file: /home/projects/sveltejs-kit-template-default-4wrfgp/src/routes/__layout.svelte:1:0

System Info

  System:
    OS: Linux 5.0 undefined
    CPU: (4) x64 Intel(R) Core(TM) i9-9880H CPU @ 2.30GHz
    Memory: 0 Bytes / 0 Bytes
    Shell: 1.0 - /bin/jsh
  Binaries:
    Node: 16.14.2 - /usr/local/bin/node
    Yarn: 1.22.10 - /bin/yarn
    npm: 7.17.0 - /bin/npm
  npmPackages:
    @sveltejs/adapter-auto: next => 1.0.0-next.53 
    @sveltejs/kit: next => 1.0.0-next.359 
    svelte: ^3.46.0 => 3.48.0 
    vite: ^2.9.13 => 2.9.13

Severity

blocking an upgrade

Additional Information

No response

@Conduitry Conduitry added the bug Something isn't working label Jul 5, 2022
@Conduitry Conduitry changed the title Conditional configuration in vite.config.js breaks build Conditional configuration in vite.config.js breaks prod build Jul 5, 2022
@Conduitry
Copy link
Member Author

The second argument configEnv to the config method in the Vite plugin object needs to find its way into the different places the get_vite_config is called from, and we need to use the configEnv if vite.config.js exports a function.

@Conduitry
Copy link
Member Author

Actually, @benmccann is there a reason that build_server.js and build_service_worker.js try to manually load the Vite config at all? Couldn't they just be passed the config argument that's already passed to the config method in the Vite plugin? It looks like this already takes care of the config resolution for us, including calling the function with the ConfigEnv value if the config exports a function.

@Conduitry
Copy link
Member Author

This is what I have right now, and it seems to be working okay so far. vite_config is passed into build_server and build_service_worker, who no longer load the config themselves.

diff --git a/packages/kit/src/vite/build/build_server.js b/packages/kit/src/vite/build/build_server.js
index f5b56ab3..cac9f04a 100644
--- a/packages/kit/src/vite/build/build_server.js
+++ b/packages/kit/src/vite/build/build_server.js
@@ -1,7 +1,7 @@
 import fs from 'fs';
 import path from 'path';
 import { mkdirp, posixify } from '../../utils/filesystem.js';
-import { get_vite_config, merge_vite_configs, resolve_entry } from '../utils.js';
+import { merge_vite_configs, resolve_entry } from '../utils.js';
 import { load_template } from '../../core/config/index.js';
 import { get_runtime_path } from '../../core/utils.js';
 import { create_build, find_deps, get_default_config, remove_svelte_kit } from './utils.js';
@@ -107,6 +107,7 @@ export class Server {
  * @param {{
  *   cwd: string;
  *   config: import('types').ValidatedConfig
+ *   vite_config: import('vite').UserConfig
  *   manifest_data: import('types').ManifestData
  *   build_dir: string;
  *   output_dir: string;
@@ -115,7 +116,7 @@ export class Server {
  * @param {{ vite_manifest: import('vite').Manifest, assets: import('rollup').OutputAsset[] }} client
  */
 export async function build_server(options, client) {
-	const { cwd, config, manifest_data, build_dir, output_dir, service_worker_entry_file } = options;
+	const { cwd, config, vite_config, manifest_data, build_dir, output_dir, service_worker_entry_file } = options;
 
 	let hooks_file = resolve_entry(config.kit.files.hooks);
 	if (!hooks_file || !fs.existsSync(hooks_file)) {
@@ -174,8 +175,6 @@ export async function build_server(options, client) {
 		})
 	);
 
-	const vite_config = await get_vite_config();
-
 	const merged_config = merge_vite_configs(
 		get_default_config({ config, input, ssr: true, outDir: `${output_dir}/server` }),
 		vite_config
diff --git a/packages/kit/src/vite/build/build_service_worker.js b/packages/kit/src/vite/build/build_service_worker.js
index 747f3696..cb1f293a 100644
--- a/packages/kit/src/vite/build/build_service_worker.js
+++ b/packages/kit/src/vite/build/build_service_worker.js
@@ -1,13 +1,14 @@
 import fs from 'fs';
 import * as vite from 'vite';
 import { s } from '../../utils/misc.js';
-import { get_vite_config, merge_vite_configs } from '../utils.js';
+import { merge_vite_configs } from '../utils.js';
 import { normalize_path } from '../../utils/url.js';
 import { assets_base, remove_svelte_kit } from './utils.js';
 
 /**
  * @param {{
  *   config: import('types').ValidatedConfig;
+ *   vite_config: import('vite').UserConfig;
  *   manifest_data: import('types').ManifestData;
  *   output_dir: string;
  *   service_worker_entry_file: string | null;
@@ -65,7 +66,6 @@ export async function build_service_worker(
 			.trim()
 	);
 
-	const vite_config = await get_vite_config();
 	const merged_config = merge_vite_configs(vite_config, {
 		base: assets_base(config.kit),
 		build: {
diff --git a/packages/kit/src/vite/index.js b/packages/kit/src/vite/index.js
index 760c0feb..3c17edf5 100644
--- a/packages/kit/src/vite/index.js
+++ b/packages/kit/src/vite/index.js
@@ -233,6 +233,7 @@ function kit() {
 			const options = {
 				cwd,
 				config: svelte_config,
+				vite_config,
 				build_dir: paths.build_dir, // TODO just pass `paths`
 				manifest_data,
 				output_dir: paths.output_dir,
diff --git a/packages/kit/src/vite/utils.js b/packages/kit/src/vite/utils.js
index 08c440ce..4e507ef5 100644
--- a/packages/kit/src/vite/utils.js
+++ b/packages/kit/src/vite/utils.js
@@ -1,21 +1,7 @@
 import fs from 'fs';
 import path from 'path';
-import { pathToFileURL } from 'url';
 import { get_runtime_path } from '../core/utils.js';
 
-/**
- * @return {Promise<import('vite').UserConfig>}
- */
-export async function get_vite_config() {
-	for (const file of ['vite.config.js', 'vite.config.mjs', 'vite.config.cjs']) {
-		if (fs.existsSync(file)) {
-			const config = await import(pathToFileURL(file).toString());
-			return config.default || config;
-		}
-	}
-	throw new Error('Could not find vite.config.js');
-}
-
 /**
  * @param {...import('vite').UserConfig} configs
  * @returns {import('vite').UserConfig}

@benmccann
Copy link
Member

Actually, @benmccann is there a reason that build_server.js and build_service_worker.js try to manually load the Vite config at all? Couldn't they just be passed the config argument that's already passed to the config method in the Vite plugin?

Sadly not. An older version did that, which led to issues. See #5308

@Conduitry
Copy link
Member Author

Bummer. All right, I've opened a PR that keeps the get_vite_config function but now passes it a vite_config_env object.

Rich-Harris added a commit to Conduitry/kit that referenced this issue Jul 6, 2022
Rich-Harris added a commit that referenced this issue Jul 6, 2022
* fix loading of conditional Vite configs (#5372)

* add changeset

* use loadConfigFromFile to load Vite config

* Update .changeset/poor-knives-yawn.md

Co-authored-by: Rich Harris <hello@rich-harris.dev>
Co-authored-by: Rich Harris <richard.a.harris@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants