fix(constraints): make constraints --fix persist changes to manifest (yarnpkg#2245)

* fix(constraints): make constraints --fix persist changes to manifest

The change introduced in yarnpkg#1775 to fix fixing the field constraints
broke fixing dependency constraints, by overriding the changes made
to the manifest before persisting.

Fixes yarnpkg#2242

* fix(constraints): add tests to constraints --fix

Author: bgotink

.yarn/versions/273ab9b6.yml

@@ -0,0 +1,2 @@
+releases:
+  "@yarnpkg/plugin-constraints": patch

packages/acceptance-tests/pkg-tests-specs/sources/commands/constraints/fix.test.js

@@ -0,0 +1,58 @@
+import {xfs} from '@yarnpkg/fslib';
+
+describe(`Commands`, () => {
+  const config = {
+    plugins: [
+      require.resolve(`@yarnpkg/monorepo/scripts/plugin-constraints.js`),
+    ],
+  };
+
+  const manifest = {
+    dependencies: {
+      'is-number': `1.0.0`,
+    },
+    license: `MIT`,
+  };
+
+  describe(`constraints --fix`, () => {
+    test(`test apply fix to dependencies`, makeTemporaryEnv(manifest, config, async ({path, run, source}) => {
+      await xfs.writeFilePromise(`${path}/constraints.pro`, `
+      gen_enforced_dependency('.', 'is-number', '2.0.0', dependencies).
+      `);
+
+      await run(`constraints`, `--fix`);
+
+      const fixedManifest = await xfs.readJsonPromise(`${path}/package.json`);
+
+      expect(fixedManifest.dependencies[`is-number`]).toBe(`2.0.0`);
+      expect(fixedManifest.license).toBe(`MIT`);
+    }));
+
+    test(`test apply fix to fields`, makeTemporaryEnv(manifest, config, async ({path, run, source}) => {
+      await xfs.writeFilePromise(`${path}/constraints.pro`, `
+      gen_enforced_field('.', 'license', 'BSD-2-Clause').
+      `);
+
+      await run(`constraints`, `--fix`);
+
+      const fixedManifest = await xfs.readJsonPromise(`${path}/package.json`);
+
+      expect(fixedManifest.dependencies[`is-number`]).toBe(`1.0.0`);
+      expect(fixedManifest.license).toBe(`BSD-2-Clause`);
+    }));
+
+    test(`test apply fix to fields and manifests`, makeTemporaryEnv(manifest, config, async ({path, run, source}) => {
+      await xfs.writeFilePromise(`${path}/constraints.pro`, `
+      gen_enforced_dependency('.', 'is-number', '2.0.0', dependencies).
+      gen_enforced_field('.', 'license', 'BSD-2-Clause').
+      `);
+
+      await run(`constraints`, `--fix`);
+
+      const fixedManifest = await xfs.readJsonPromise(`${path}/package.json`);
+
+      expect(fixedManifest.dependencies[`is-number`]).toBe(`2.0.0`);
+      expect(fixedManifest.license).toBe(`BSD-2-Clause`);
+    }));
+  });
+});

packages/plugin-constraints/sources/commands/constraints.ts

@@ -49,39 +49,45 @@ export default class ConstraintsCheckCommand extends BaseCommand {
       for (let t = 0, T = this.fix ? 10 : 1; t < T; ++t) {
         errors = [];
 
-        const toSave = new Set<Workspace>();
         const result = await constraints.process();
 
-        await processDependencyConstraints(toSave, errors, result.enforcedDependencies, {
+        const modifiedDependencies = new Set<Workspace>();
+        await processDependencyConstraints(modifiedDependencies, errors, result.enforcedDependencies, {
           fix: this.fix,
           configuration,
         });
 
-        await processFieldConstraints(toSave, errors, result.enforcedFields, {
+        // Dependency constraints work on the manifess, field constraints work on the raw JSON objects
+        for (const {manifest} of modifiedDependencies)
+          manifest.exportTo(manifest.raw = {});
+
+        const modifiedFields = new Set<Workspace>();
+        await processFieldConstraints(modifiedFields, errors, result.enforcedFields, {
           fix: this.fix,
           configuration,
         });
 
+        // Persist changes made by the constraints back to the Manifest instance
+        for (const {manifest} of modifiedFields)
+          manifest.load(manifest.raw);
+
         allSaves = new Set([
           ...allSaves,
-          ...toSave,
+          ...modifiedDependencies,
+          ...modifiedFields,
         ]);
 
         // If we didn't apply any change then we can exit the loop
-        if (toSave.size === 0) {
+        if (modifiedDependencies.size === 0 && modifiedFields.size === 0) {
           break;
         }
       }
 
       // save all modified manifests
       await Promise.all([...allSaves].map(async workspace => {
-        // Constraints modify the raw manifest so we need to reload it here
-        // otherwise changes are not persisted
-        workspace.manifest.load(workspace.manifest.raw);
         await workspace.persistManifest();
       }));
 
-
       // report all outstanding errors
       for (const [messageName, message] of errors) {
         report.reportError(messageName, message);