warn when a plugin's packages would block its own updates
update is a git pull and then the ordinary install path, so the npm step is picked up for free — a plugin that adds a dependency in a later version gets it installed on update, with no second implementation to drift. but pullPlugin refuses on ANY uncommitted change, and node_modules/ that nothing ignores is one. so a plugin shipping package.json without ignoring it installs perfectly once and can then never be updated again: the button reports "N uncommitted changes" forever, about a directory the platform itself created, and nobody seeing that message has a reason to connect it to a package.json from three versions ago. every plugin that exists today ignores node_modules, which is luck rather than a rule — this makes it a checked one. asked with `git check-ignore` rather than by reading .gitignore: the rule may be nested, global, or in .git/info/exclude, and all three count. warned, not fixed. writing into the plugin's own .gitignore would be the platform editing a repository it does not own, and would itself dirty the checkout — which is the exact thing being avoided.
This commit is contained in:
@@ -24,7 +24,7 @@ import { generatePluginSchemas, pushPluginSchema } from './schema';
|
||||
import { declaredTables, dropTables, existingTables } from './tables';
|
||||
import type { OnLog } from './run-logged';
|
||||
import { installDependencies, installRelease, manualInstallHint, reportDependencies, stillMissing } from './os-deps';
|
||||
import { installNpmDependencies, npmManifestFor, verifyNpmDependencies } from './npm-deps';
|
||||
import { installNpmDependencies, npmManifestFor, npmWouldBlockUpdates, verifyNpmDependencies } from './npm-deps';
|
||||
import { mountPrefix, type DiscoveredPlugin } from './manifest';
|
||||
|
||||
// The install runner: the four verbs, each as a short ordered list of effects.
|
||||
@@ -295,6 +295,19 @@ export async function installPlugin(
|
||||
};
|
||||
}
|
||||
await step(steps, onStep, 'packages: installed and resolvable');
|
||||
|
||||
// Said now, while there is something to be done about it. `pullPlugin` refuses on any uncommitted
|
||||
// change, so an unignored `node_modules/` turns the Update button into a permanent "N uncommitted
|
||||
// changes" about a directory the platform created — and the person seeing that message has no
|
||||
// reason to connect it to a package.json shipped three versions ago.
|
||||
if (await npmWouldBlockUpdates(plugin)) {
|
||||
await step(
|
||||
steps,
|
||||
onStep,
|
||||
'packages: WARNING — this plugin does not ignore node_modules/, so Update will refuse as dirty. ' +
|
||||
'Add `node_modules/` to its .gitignore.',
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
if (plugin.schema) {
|
||||
|
||||
@@ -40,6 +40,38 @@ export const npmManifestFor = (plugin: DiscoveredPlugin): string | null => {
|
||||
|
||||
export type NpmInstallResult = { ok: boolean; ran: boolean; output: string };
|
||||
|
||||
/**
|
||||
* Will the packages this is about to install make the plugin un-updatable?
|
||||
*
|
||||
* `pullPlugin` refuses on ANY uncommitted change — `git status --porcelain` non-empty — because it is
|
||||
* about to `merge --ff-only` over the checkout and will not overwrite someone's work. `node_modules/`
|
||||
* that nothing ignores is an uncommitted change, so a plugin shipping `package.json` without ignoring it
|
||||
* installs perfectly once and can then never be updated again. The Update button just says "N uncommitted
|
||||
* changes" forever, about a directory the platform itself created.
|
||||
*
|
||||
* Asked with `git check-ignore`, not by reading `.gitignore`: the rule may be nested, global, or in
|
||||
* `.git/info/exclude`, and every one of those counts.
|
||||
*
|
||||
* Not fixed automatically. Writing a line into the plugin's own `.gitignore` would be the platform
|
||||
* editing a repository it does not own — and would itself dirty the checkout, which is the very thing
|
||||
* being avoided.
|
||||
*/
|
||||
export async function npmWouldBlockUpdates(plugin: DiscoveredPlugin): Promise<boolean> {
|
||||
if (!npmManifestFor(plugin)) return false;
|
||||
if (!existsSync(join(plugin.dir, '.git'))) return false; // not a checkout — nothing to update from
|
||||
try {
|
||||
const proc = Bun.spawn(['git', 'check-ignore', '-q', 'node_modules'], {
|
||||
cwd: plugin.dir,
|
||||
stdout: 'ignore',
|
||||
stderr: 'ignore',
|
||||
});
|
||||
// 0 = ignored, 1 = not ignored, anything else = git could not answer, which is not a finding.
|
||||
return (await proc.exited) === 1;
|
||||
} catch {
|
||||
return false;
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Install a plugin's packages, in its own directory.
|
||||
*
|
||||
|
||||
@@ -3,6 +3,8 @@ import { mkdirSync, rmSync, writeFileSync } from 'node:fs';
|
||||
import { join } from 'node:path';
|
||||
import { PLUGINS_DIR } from './discover';
|
||||
import { pullPlugin } from './update';
|
||||
import { discoverPlugins } from './discover';
|
||||
import { npmManifestFor, npmWouldBlockUpdates } from './npm-deps';
|
||||
|
||||
// The REFUSALS are what this test is for. A pull that works is easy to believe; a pull that quietly
|
||||
// discarded somebody's uncommitted work is the failure that matters, and every plugin directory on a
|
||||
@@ -53,3 +55,19 @@ describe('pullPlugin refuses rather than guesses', () => {
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
describe('npm packages and the Update button', () => {
|
||||
// The trap: `pullPlugin` refuses on ANY uncommitted change, and `node_modules/` that nothing ignores
|
||||
// is one. So a plugin shipping `package.json` without ignoring it installs perfectly once and can then
|
||||
// never be updated — the button says "N uncommitted changes" forever, about a directory the platform
|
||||
// itself created. Every plugin that exists today ignores it, which is luck rather than a rule.
|
||||
test('atelier — the only plugin with packages — ignores node_modules', async () => {
|
||||
const { plugins } = await discoverPlugins();
|
||||
const withPackages = plugins.filter((p) => npmManifestFor(p));
|
||||
|
||||
// Not asserting that atelier specifically is installed — a checkout without it is legitimate.
|
||||
for (const plugin of withPackages) {
|
||||
expect(await npmWouldBlockUpdates(plugin), `${plugin.appName} would block its own updates`).toBe(false);
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user