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

feat: auto-exclude svelte dependencies in vite.optimizeDeps #145

Merged
merged 10 commits into from
Aug 20, 2021
5 changes: 5 additions & 0 deletions .changeset/four-chairs-rhyme.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@sveltejs/vite-plugin-svelte': minor
---

automatically exclude svelte dependencies in vite.optimizeDeps
9 changes: 8 additions & 1 deletion .eslintrc.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,8 @@ module.exports = {
ecmaVersion: 2020
},
rules: {
'no-debugger': ['error'],
'no-console': 'off',
'no-debugger': 'error',
'node/no-missing-import': [
'error',
{
Expand Down Expand Up @@ -58,6 +59,12 @@ module.exports = {
'no-process-exit': 'off'
},
overrides: [
{
files: ['packages/vite-plugin-svelte/src/**'],
rules: {
'no-console': 'error'
}
},
{
files: ['packages/e2e-tests/**', 'packages/playground/**'],
rules: {
Expand Down
3 changes: 2 additions & 1 deletion packages/e2e-tests/hmr-test-dependency/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -2,5 +2,6 @@
"version": "1.0.0",
"private": true,
"name": "e2e-tests-hmr-test-dependency",
"main": "index.js"
"main": "index.js",
"svelte": "index.js"
}
3 changes: 3 additions & 0 deletions packages/e2e-tests/test-dependency-svelte-field/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -9,5 +9,8 @@
],
"exports": {
"./package.json": "./package.json"
},
"dependencies": {
"e2e-tests-hmr-test-dependency": "workspace:*"
}
}
3 changes: 0 additions & 3 deletions packages/playground/optimizedeps-include/vite.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,6 @@ const SVELTE_IMPORTS = [
export default defineConfig(({ command, mode }) => {
const isProduction = mode === 'production';
return {
optimizeDeps: {
include: [...SVELTE_IMPORTS]
},
plugins: [svelte()],
build: {
minify: isProduction
Expand Down
2 changes: 1 addition & 1 deletion packages/vite-plugin-svelte/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@
"./package.json": "./package.json"
},
"scripts": {
"dev": "pnpm run build:ci -- --watch src",
"dev": "pnpm run build:ci -- --sourcemap --watch src",
"build:ci": "rimraf dist && tsup-node src/index.ts --format esm,cjs --no-splitting",
"build": "pnpm run build:ci -- --dts --sourcemap"
},
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
import { findRootSvelteDependencies } from '../dependencies';
import * as path from 'path';

describe('dependencies', () => {
describe('findRootSvelteDependencies', () => {
it('should find svelte dependencies in packages/e2e-test/hmr', async () => {
const deps = findRootSvelteDependencies(path.resolve('packages/e2e-tests/hmr'));
expect(deps).toHaveLength(1);
expect(deps[0].name).toBe('e2e-tests-hmr-test-dependency');
expect(deps[0].path).toEqual([]);
});
it('should find nested svelte dependencies in packages/e2e-test/package-json-svelte-field', async () => {
const deps = findRootSvelteDependencies(
path.resolve('packages/e2e-tests/package-json-svelte-field')
);
expect(deps).toHaveLength(2);
expect(deps[0].name).toBe('e2e-tests-test-dependency-svelte-field');
expect(deps[1].name).toBe('e2e-tests-hmr-test-dependency');
expect(deps[1].path).toEqual(['e2e-tests-test-dependency-svelte-field']);
});
});
});
190 changes: 190 additions & 0 deletions packages/vite-plugin-svelte/src/utils/dependencies.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,190 @@
import { log } from './log';
import path from 'path';
import fs from 'fs';
import { createRequire } from 'module';

export function findRootSvelteDependencies(root: string, cwdFallback = true): SvelteDependency[] {
log.debug(`findSvelteDependencies: searching svelte dependencies in ${root}`);
const pkgFile = path.join(root, 'package.json');
if (!fs.existsSync(pkgFile)) {
if (cwdFallback) {
const cwd = process.cwd();
if (root !== cwd) {
log.debug(`no package.json found in vite root ${root}`);
return findRootSvelteDependencies(cwd, false);
}
}
log.warn(`no package.json found, findRootSvelteDependencies failed`);
return [];
}

const pkg = parsePkg(root);
if (!pkg) {
return [];
}

const deps = [
...Object.keys(pkg.dependencies || {}),
...Object.keys(pkg.devDependencies || {})
].filter((dep) => !is_common_without_svelte_field(dep));

return getSvelteDependencies(deps, root);
}

function getSvelteDependencies(
deps: string[],
pkgDir: string,
path: string[] = []
): SvelteDependency[] {
const result = [];
const localRequire = createRequire(`${pkgDir}/package.json`);
const resolvedDeps = deps
.map((dep) => resolveSvelteDependency(dep, localRequire))
.filter(Boolean);
// @ts-ignore
for (const { pkg, dir } of resolvedDeps) {
result.push({ name: pkg.name, pkg, dir, path });
if (pkg.dependencies) {
let dependencyNames = Object.keys(pkg.dependencies);
const circular = dependencyNames.filter((name) => path.includes(name));
if (circular.length > 0) {
log.warn.enabled &&
log.warn(
`skipping circular svelte dependencies in automated vite optimizeDeps handling`,
circular.map((x) => path.concat(x).join('>'))
);
dependencyNames = dependencyNames.filter((name) => !path.includes(name));
}
if (path.length === 3) {
log.warn.once(`encountered deep svelte dependency tree: ${path.join('>')}`);
}
Copy link
Member

Choose a reason for hiding this comment

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

Should we be warning when we encounter this scenario? The crawler should be able to continue down the tree, just that it's weird for this to ever happen. I think we can remove these lines, or maybe convert to debug if it helps for debugging.

Copy link
Member Author

@dominikg dominikg Aug 20, 2021

Choose a reason for hiding this comment

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

Such a deeply nested svelte component package is a sign something is afoot with the project and a warning is in order imho.

I agree that it is weird and tbh i'm hoping this never occurs in the wild. Still i'd like to know if it does and debug output would not be noticed in regular use.

To avoid spam, the warning is logged only once per run and not again if the tree is even deeper. foo>bar>baz gets logged, but foo>bar>baz>qoox won't.

Copy link
Member

Choose a reason for hiding this comment

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

I suppose it's good to know if this ever happens in the wild, but I'm concern if it would annoy the end-user since it may be something not fixable on their end. We might get an issue/pr to remove that line too. Since if it's a warning, the end-user should be able to suppress or fix it (which sometimes isn't possible).

Or we could leave it there and increment the number if someone do report it, so we know there exist an atrocity like that 😆

Copy link
Member Author

Choose a reason for hiding this comment

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

made it debug for now. I guess such a deep path would cause other issues and if it cannot be resolved with the package authors we can add back a warning (and an extra option to filter some logs?)

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good 👍

result.push(...getSvelteDependencies(dependencyNames, dir, path.concat(pkg.name)));
}
}
return result;
}

function resolveSvelteDependency(
dep: string,
localRequire: NodeRequire
): { dir: string; pkg: Pkg } | void {
try {
const pkgJson = `${dep}/package.json`;
const pkg = localRequire(pkgJson);
if (!isSvelte(pkg)) {
return;
}
const dir = path.dirname(localRequire.resolve(pkgJson));
return { dir, pkg };
} catch (e) {
log.debug.once(`dependency ${dep} does not export package.json`, e);
dominikg marked this conversation as resolved.
Show resolved Hide resolved
// walk up from default export until we find package.json with name=dep
let dir = path.dirname(localRequire.resolve(dep));
while (dir) {
const pkg = parsePkg(dir, true);
if (pkg && pkg.name === dep) {
if (!isSvelte(pkg)) {
return;
}
log.warn.once(
`package.json of ${dep} has a "svelte" field but does not include itself in exports field. Please ask package maintainer to update`
);
return { dir, pkg };
}
const parent = path.dirname(dir);
if (parent === dir) {
break;
}
dir = parent;
}
}
log.debug.once(`failed to resolve ${dep}`);
}

function parsePkg(dir: string, silent = false): Pkg | void {
const pkgFile = path.join(dir, 'package.json');
try {
return JSON.parse(fs.readFileSync(pkgFile, 'utf-8'));
} catch (e) {
!silent && log.warn.enabled && log.warn(`failed to parse ${pkgFile}`, e);
}
}

function isSvelte(pkg: Pkg) {
return !!pkg.svelte;
}

const COMMON_DEPENDENCIES_WITHOUT_SVELTE_FIELD = [
Copy link
Member

Choose a reason for hiding this comment

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

should we include everything that comes with a new SvelteKit project?

  "devDependencies": {
    "@sveltejs/kit": "next",
    "@types/cookie": "^0.4.0",
    "@typescript-eslint/eslint-plugin": "^4.19.0",
    "@typescript-eslint/parser": "^4.19.0",
    "eslint": "^7.22.0",
    "eslint-config-prettier": "^8.1.0",
    "eslint-plugin-svelte3": "^3.2.0",
    "prettier": "~2.2.1",
    "prettier-plugin-svelte": "^2.2.0",
    "svelte": "^3.34.0",
    "svelte-check": "^2.0.0",
    "svelte-preprocess": "^4.0.0",
    "tslib": "^2.0.0",
    "typescript": "^4.0.0"
  },
  "type": "module",
  "dependencies": {
    "@fontsource/fira-mono": "^4.2.2",
    "@lukeed/uuid": "^2.0.0",
    "cookie": "^0.4.1"
  }

Copy link
Member Author

Choose a reason for hiding this comment

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

most of it was covered already by the prefix checks, added the remaining ones. thanks!!

'@lukeed/uuid',
'@sveltejs/vite-plugin-svelte',
'@sveltejs/kit',
'autoprefixer',
'cookie',
'dotenv',
'esbuild',
'eslint',
'jest',
'mdsvex',
'postcss',
'prettier',
'svelte',
'svelte-check',
'svelte-hmr',
'svelte-preprocess',
'tslib',
'typescript',
'vite'
];
const COMMON_PREFIXES_WITHOUT_SVELTE_FIELD = [
'@fontsource/',
'@postcss-plugins/',
'@rollup/',
'@sveltejs/adapter-',
'@types/',
'@typescript-eslint/',
'eslint-',
'jest-',
'postcss-plugin-',
'prettier-plugin-',
'rollup-plugin-',
'vite-plugin-'
];

/**
* Test for common dependency names that tell us it is not a package including a svelte field, eg. eslint + plugins.
*
* This speeds up the find process as we don't have to try and require the package.json for all of them
*
* @param dependency {string}
* @returns {boolean} true if it is a dependency without a svelte field
*/
function is_common_without_svelte_field(dependency: string): boolean {
return (
COMMON_DEPENDENCIES_WITHOUT_SVELTE_FIELD.includes(dependency) ||
COMMON_PREFIXES_WITHOUT_SVELTE_FIELD.some(
(prefix) =>
prefix.startsWith('@')
? dependency.startsWith(prefix)
: dependency.substring(dependency.lastIndexOf('/') + 1).startsWith(prefix) // check prefix omitting @scope/
)
);
}

export interface SvelteDependency {
name: string;
dir: string;
pkg: Pkg;
path: string[];
}

export interface Pkg {
name: string;
svelte?: string;
dependencies?: DependencyList;
devDependencies?: DependencyList;
[key: string]: any;
}

export interface DependencyList {
[key: string]: string;
}
2 changes: 1 addition & 1 deletion packages/vite-plugin-svelte/src/utils/log.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
/* eslint-disable no-unused-vars */
/* eslint-disable no-unused-vars,no-console */
import { cyan, yellow, red } from 'kleur/colors';
import debug from 'debug';
import { ResolvedOptions, Warning } from './options';
Expand Down
63 changes: 46 additions & 17 deletions packages/vite-plugin-svelte/src/utils/options.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@ import {
// eslint-disable-next-line node/no-missing-import
} from 'svelte/types/compiler/preprocess';
import path from 'path';
import { findRootSvelteDependencies } from './dependencies';
import { DepOptimizationOptions } from 'vite/src/node/optimizer/index';

const knownOptions = new Set([
'configFile',
Expand Down Expand Up @@ -179,24 +181,8 @@ export function buildExtraViteConfig(
options: ResolvedOptions,
config: UserConfig
): Partial<UserConfig> {
// include svelte imports for optimization unless explicitly excluded
const include: string[] = [];
const exclude: string[] = ['svelte-hmr'];
const isSvelteExcluded = config.optimizeDeps?.exclude?.includes('svelte');
if (!isSvelteExcluded) {
const svelteImportsToInclude = SVELTE_IMPORTS.filter((x) => x !== 'svelte/ssr'); // not used on clientside
log.debug(
`adding bare svelte packages to optimizeDeps.include: ${svelteImportsToInclude.join(', ')} `
);
include.push(...svelteImportsToInclude);
} else {
log.debug('"svelte" is excluded in optimizeDeps.exclude, skipped adding it to include.');
}
const extraViteConfig: Partial<UserConfig> = {
optimizeDeps: {
include,
exclude
},
optimizeDeps: buildOptimizeDepsForSvelte(options.root, config.optimizeDeps),
resolve: {
mainFields: [...SVELTE_RESOLVE_MAIN_FIELDS],
dedupe: [...SVELTE_IMPORTS, ...SVELTE_HMR_IMPORTS]
Expand Down Expand Up @@ -233,6 +219,49 @@ export function buildExtraViteConfig(
return extraViteConfig;
}

function buildOptimizeDepsForSvelte(
root: string,
optimizeDeps?: DepOptimizationOptions
): DepOptimizationOptions {
// include svelte imports for optimization unless explicitly excluded
const include: string[] = [];
const exclude: string[] = ['svelte-hmr'];
const isSvelteExcluded = optimizeDeps?.exclude?.includes('svelte');
if (!isSvelteExcluded) {
const svelteImportsToInclude = SVELTE_IMPORTS.filter((x) => x !== 'svelte/ssr'); // not used on clientside
log.debug(
`adding bare svelte packages to optimizeDeps.include: ${svelteImportsToInclude.join(', ')} `
);
include.push(...svelteImportsToInclude);
} else {
log.debug('"svelte" is excluded in optimizeDeps.exclude, skipped adding it to include.');
}

// extra handling for svelte dependencies in the project
const svelteDeps = findRootSvelteDependencies(root);
const svelteDepsToExclude = Array.from(new Set(svelteDeps.map((dep) => dep.name))).filter(
(dep) => !optimizeDeps?.include?.includes(dep)
);
log.debug(`automatically excluding found svelte dependencies: ${svelteDepsToExclude.join(', ')}`);
exclude.push(...svelteDepsToExclude);

/* // TODO enable once https://github.com/vitejs/vite/pull/4634 lands
const transitiveDepsToInclude = svelteDeps
.filter((dep) => svelteDepsToExclude.includes(dep.name))
.flatMap((dep) =>
Object.keys(dep.pkg.dependencies || {})
.filter((depOfDep) => !svelteDepsToExclude.includes(depOfDep))
.map((depOfDep) => dep.path.concat(depOfDep).join('>'))
);
log.debug(
`reincluding transitive dependencies of excluded svelte dependencies`,
transitiveDepsToInclude
);
include.push(...transitiveDepsToInclude);
*/
return { include, exclude };
}

export interface Options {
// eslint-disable no-unused-vars
/** path to svelte config file, either absolute or relative to vite root*/
Expand Down
5 changes: 4 additions & 1 deletion pnpm-lock.yaml

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