Skip to content
Closed
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/weak-lamps-shake.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'sv': patch
---

feat(add): install prompt only if needed
8 changes: 6 additions & 2 deletions packages/cli/commands/add/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -541,7 +541,11 @@ export async function runAddCommand(
const details = officialDetails.concat(commDetails);

const addonMap: AddonMap = Object.assign({}, ...details.map((a) => ({ [a.id]: a })));
const { filesToFormat, pnpmBuildDependencies: addonPnpmBuildDependencies } = await applyAddons({
const {
filesToFormat,
pnpmBuildDependencies: addonPnpmBuildDependencies,
installNeeded
} = await applyAddons({
workspace,
addonSetupResults,
addons: addonMap,
Expand All @@ -552,7 +556,7 @@ export async function runAddCommand(

// prompt for package manager and install dependencies
let packageManager: PackageManager | undefined;
if (options.install) {
if (installNeeded && options.install) {
packageManager =
options.install === true ? await packageManagerPrompt(options.cwd) : options.install;

Expand Down
30 changes: 24 additions & 6 deletions packages/cli/commands/add/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import pc from 'picocolors';
import { exec } from 'tinyexec';
import { parseJson } from '@sveltejs/cli-core/parsers';
import { resolveCommand, type AgentName } from 'package-manager-detector';
import type { Highlighter, Workspace } from '@sveltejs/cli-core';
import { isVersionAtLeast, type Highlighter, type Workspace } from '@sveltejs/cli-core';

export type Package = {
name: string;
Expand Down Expand Up @@ -57,27 +57,45 @@ export function readFile(cwd: string, filePath: string): string {
return text;
}

const checkInstallNeeded = (atLeastVersion: string, version: string | undefined) => {
if (!version) return true;
const atLeastVersionStripped = atLeastVersion.replace('^', '');
const versionStripped = version.replace('^', '');
return !isVersionAtLeast(versionStripped, atLeastVersionStripped);
};
Comment on lines +60 to +65
Copy link
Member

Choose a reason for hiding this comment

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

i think we may be doing too much with the inclusion of version checking. dependency versions can get very complicated with tons of edge-cases, so i'm not sure if this is really worth the effort. (note: we can get away with it with @types/node since we're targeting a very specific dependency version that we'll always know the shape/type of. we don't really have that guarantee with other 3rd party deps)

perhaps we should just show the install prompt only if there are any dependencies to be installed? although, im not sure how useful that'd be since almost every add-on tends to add some sort of new dependency

Copy link
Contributor Author

Choose a reason for hiding this comment

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

very complicated with tons of edge-cases

Don't tell me 😅, spent hours there before.


The idea is that checkInstallNeeded (and isVersionAtLeast under), do it's best for simple case, and if not will fallback with prompting. So I would say that it's helping "normal" cases and fallback to todays way.

I would totally see "Community Add-On" for scaffolding code (with a dep).


But you are already the second one saying that it might not worth the effort. (It's good to slow down my ideas from time to time :p)
I still think it's a good addition as the fallback is like today. But yes, it's a bit of code, maintenance, surface, ...


export type Dependency = { pkg: string; version: string; dev: boolean };
export function installPackages(
dependencies: Array<{ pkg: string; version: string; dev: boolean }>,
dependencies: Dependency[],
workspace: Workspace<any>
): string {
): {
pkgPath: string;
installNeeded: boolean;
} {
const { data, generateCode } = getPackageJson(workspace.cwd);

let installNeeded = false;
for (const dependency of dependencies) {
if (dependency.dev) {
data.devDependencies ??= {};
data.devDependencies[dependency.pkg] = dependency.version;
if (checkInstallNeeded(dependency.version, data.devDependencies[dependency.pkg])) {
data.devDependencies[dependency.pkg] = dependency.version;
installNeeded = true;
}
} else {
data.dependencies ??= {};
data.dependencies[dependency.pkg] = dependency.version;
if (checkInstallNeeded(dependency.version, data.dependencies[dependency.pkg])) {
data.dependencies[dependency.pkg] = dependency.version;
installNeeded = true;
}
}
}

if (data.dependencies) data.dependencies = alphabetizeProperties(data.dependencies);
if (data.devDependencies) data.devDependencies = alphabetizeProperties(data.devDependencies);

writeFile(workspace, commonFilePaths.packageJson, generateCode());
return commonFilePaths.packageJson;
return { pkgPath: commonFilePaths.packageJson, installNeeded };
}

function alphabetizeProperties(obj: Record<string, string>) {
Expand Down
27 changes: 21 additions & 6 deletions packages/cli/lib/install.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,13 @@ import { exec, NonZeroExitError } from 'tinyexec';
import { resolveCommand } from 'package-manager-detector';
import { TESTING } from '../utils/env.ts';
import { createWorkspace } from '../commands/add/workspace.ts';
import { fileExists, installPackages, readFile, writeFile } from '../commands/add/utils.ts';
import {
fileExists,
installPackages,
readFile,
writeFile,
type Dependency
} from '../commands/add/utils.ts';

export type InstallOptions<Addons extends AddonMap> = {
cwd: string;
Expand Down Expand Up @@ -54,29 +60,37 @@ export async function applyAddons({
}: ApplyAddonOptions): Promise<{
filesToFormat: string[];
pnpmBuildDependencies: string[];
installNeeded: boolean;
}> {
const filesToFormat = new Set<string>();
const allPnpmBuildDependencies: string[] = [];
let installNeeded = false;

const mapped = Object.entries(addons).map(([, addon]) => addon);
const ordered = orderAddons(mapped, addonSetupResults);

for (const addon of ordered) {
workspace = await createWorkspace({ ...workspace, options: options[addon.id] });

const { files, pnpmBuildDependencies } = await runAddon({
const {
files,
pnpmBuildDependencies,
installNeeded: addonInstallNeeded
} = await runAddon({
workspace,
addon,
multiple: ordered.length > 1
});

files.forEach((f) => filesToFormat.add(f));
pnpmBuildDependencies.forEach((s) => allPnpmBuildDependencies.push(s));
if (addonInstallNeeded) installNeeded = true;
}

return {
filesToFormat: Array.from(filesToFormat),
pnpmBuildDependencies: allPnpmBuildDependencies
pnpmBuildDependencies: allPnpmBuildDependencies,
installNeeded
};
}

Expand Down Expand Up @@ -123,7 +137,7 @@ async function runAddon({ addon, multiple, workspace }: RunAddon) {
}
}

const dependencies: Array<{ pkg: string; version: string; dev: boolean }> = [];
const dependencies: Dependency[] = [];
const pnpmBuildDependencies: string[] = [];
const sv: SvApi = {
file: (path, content) => {
Expand Down Expand Up @@ -179,12 +193,13 @@ async function runAddon({ addon, multiple, workspace }: RunAddon) {
};
await addon.run({ ...workspace, sv });

const pkgPath = installPackages(dependencies, workspace);
const { pkgPath, installNeeded } = installPackages(dependencies, workspace);
files.add(pkgPath);

return {
files: Array.from(files),
pnpmBuildDependencies
pnpmBuildDependencies,
installNeeded
};
}

Expand Down
6 changes: 6 additions & 0 deletions packages/core/common.ts
Original file line number Diff line number Diff line change
Expand Up @@ -46,3 +46,9 @@ export function isVersionUnsupportedBelow(

return undefined;
}

export function isVersionAtLeast(versionStr: string, atLeastStr: string): boolean {
const unsupported = isVersionUnsupportedBelow(versionStr, atLeastStr);
// Let's be conservative and assume that if we can't say anything, it's not at least
return unsupported === undefined ? false : !unsupported;
}
2 changes: 1 addition & 1 deletion packages/core/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ export { log } from '@clack/prompts';
export { default as colors } from 'picocolors';
export { default as dedent } from 'dedent';
export * as utils from './utils.ts';
export { isVersionUnsupportedBelow, splitVersion } from './common.ts';
export { isVersionUnsupportedBelow, splitVersion, isVersionAtLeast } from './common.ts';

export type * from './addon/processors.ts';
export type * from './addon/options.ts';
Expand Down
19 changes: 18 additions & 1 deletion packages/core/tests/common.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { expect, describe, it } from 'vitest';
import { splitVersion, isVersionUnsupportedBelow } from '../common.ts';
import { splitVersion, isVersionUnsupportedBelow, isVersionAtLeast } from '../common.ts';

describe('versionSplit', () => {
const combinationsVersionSplit = [
Expand Down Expand Up @@ -46,3 +46,20 @@ describe('minimumRequirement', () => {
}
);
});

describe('isVersionAtLeast', () => {
const combinationsIsVersionAtLeast = [
{ version: '1.0.0', atLeast: '1.0.0', expected: true },
{ version: '1.0.0', atLeast: '1.0.1', expected: false },
{ version: '1.0.1', atLeast: '1.0.0', expected: true },
{ version: '1.0.1', atLeast: '1', expected: true },
{ version: '1', atLeast: '1', expected: true },
{ version: '*', atLeast: '1', expected: false }
] as const;
it.each(combinationsIsVersionAtLeast)(
'($version atLeast $atLeast) should be $expected',
({ version, atLeast, expected }) => {
expect(isVersionAtLeast(version, atLeast)).toEqual(expected);
}
);
});
Loading