Skip to content

Commit

Permalink
feat: atomic package add (#234)
Browse files Browse the repository at this point in the history
Currently, when adding multiple packages using the `add` command, each will be added separately. That means, that if packages A, B and C are added and B fails for any reason, A and C will still be added to the manifest.

With this change add operations are now atomic. In the same scenario as before, the manifest would not be modified, since one of the packages could not be added.

A side-effect of this change is that `add` now only returns one error, instead of an array of them, since the first encountered error cancels the operation.

See #223
  • Loading branch information
ComradeVanti committed Apr 15, 2024
1 parent 3b75ecd commit 4b2fffc
Show file tree
Hide file tree
Showing 2 changed files with 26 additions and 11 deletions.
17 changes: 6 additions & 11 deletions src/cli/cmd-add.ts
Original file line number Diff line number Diff line change
Expand Up @@ -87,11 +87,11 @@ export type AddError =
export const add = async function (
pkgs: PackageReference | PackageReference[],
options: AddOptions
): Promise<Result<void, AddError[]>> {
): Promise<Result<void, AddError>> {
if (!Array.isArray(pkgs)) pkgs = [pkgs];
// parse env
const envResult = await parseEnv(options);
if (envResult.isErr()) return Err([envResult.error]);
if (envResult.isErr()) return envResult;
const env = envResult.value;

const client = makeNpmClient();
Expand Down Expand Up @@ -299,20 +299,15 @@ export const add = async function (
const loadResult = await tryLoadProjectManifest(env.cwd).promise;
if (loadResult.isErr()) {
logManifestLoadError(loadResult.error);
return Err([loadResult.error]);
return loadResult;
}
let manifest = loadResult.value;

// add
const errors = Array.of<AddError>();
let dirty = false;

for (const pkg of pkgs) {
const result = await tryAddToManifest(manifest, pkg);
if (result.isErr()) {
errors.push(result.error);
continue;
}
if (result.isErr()) return result;

const [newManifest, manifestChanged] = result.value;
if (manifestChanged) {
Expand All @@ -326,12 +321,12 @@ export const add = async function (
const saveResult = await trySaveProjectManifest(env.cwd, manifest).promise;
if (saveResult.isErr()) {
logManifestSaveError(saveResult.error);
return Err([saveResult.error]);
return saveResult;
}

// print manifest notice
log.notice("", "please open Unity project to apply changes");
}

return errors.length === 0 ? Ok(undefined) : Err(errors);
return Ok(undefined);
};
20 changes: 20 additions & 0 deletions test/cmd-add.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -468,5 +468,25 @@ describe("cmd-add.ts", () => {
expect(addResult).toBeOk();
expect(warnSpy).toHaveLogLike("package.unity", "2020 is not valid");
});

it("should perform multiple adds atomically", async () => {
mockResolvedPackuments(
[exampleRegistryUrl, remotePackumentA],
[exampleRegistryUrl, remotePackumentB]
);

const addResult = await add(
// Second package will fail.
// Because of this the whole operation should fail.
[packageA, packageMissing, packageB],
upstreamOptions
);

expect(addResult).toBeError();
// Since not all packages could be added, the manifest should not be modified.
await mockProject.tryAssertManifest((manifest) =>
expect(manifest).toEqual(defaultManifest)
);
});
});
});

0 comments on commit 4b2fffc

Please sign in to comment.