From 56ee288d34466fd0e231ed70334992481d982a9a Mon Sep 17 00:00:00 2001
From: Titus Wormer <tituswormer@gmail.com>
Date: Mon, 14 Aug 2023 13:03:14 +0200
Subject: [PATCH] Fix non-first parameter merging when reconfiguring plugins
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

If you configure a plugin with multiple parameters (which is supported
but not particularly recommended, or used), and then reconfigure it
multiple times, the non-first parameters (so everything other than
what’s typically the `options` object), we now take the last configured
values.
Previously, earlier values remained.
---
 lib/index.js | 27 +++++++++---------
 test/use.js  | 79 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 92 insertions(+), 14 deletions(-)

diff --git a/lib/index.js b/lib/index.js
index efb95eb4..71c3cff6 100644
--- a/lib/index.js
+++ b/lib/index.js
@@ -1210,29 +1210,28 @@ export class Processor extends CallableInstance {
      */
     function addPlugin(plugin, parameters) {
       let index = -1
-      /** @type {PluginTuple<Array<unknown>> | undefined} */
-      let entry
+      let entryIndex = -1
 
       while (++index < attachers.length) {
         if (attachers[index][0] === plugin) {
-          entry = attachers[index]
+          entryIndex = index
           break
         }
       }
 
-      if (entry) {
-        let value = parameters[0]
-        if (isPlainObj(entry[1]) && isPlainObj(value)) {
-          value = structuredClone({...entry[1], ...value})
+      if (entryIndex === -1) {
+        attachers.push([plugin, ...parameters])
+      }
+      // Only set if there was at least a `primary` value, otherwise we’d change
+      // `arguments.length`.
+      else if (parameters.length > 0) {
+        let [primary, ...rest] = parameters
+        const currentPrimary = attachers[entryIndex][1]
+        if (isPlainObj(currentPrimary) && isPlainObj(primary)) {
+          primary = structuredClone({...currentPrimary, ...primary})
         }
 
-        entry[1] = value
-        // To do: should the rest be set?
-      } else {
-        // It’s important to pass arguments, because an explicit passed
-        // `undefined` is different from a not-passed `value`.
-        // This way we keep things at their place.
-        attachers.push([plugin, ...parameters])
+        attachers[entryIndex] = [plugin, primary, ...rest]
       }
     }
   }
diff --git a/test/use.js b/test/use.js
index 4f0f4523..77e00b54 100644
--- a/test/use.js
+++ b/test/use.js
@@ -1291,4 +1291,83 @@ test('`use`', async function (t) {
       }
     )
   })
+
+  await t.test('reconfigure (non-first parameters)', async function (t) {
+    await t.test(
+      'should reconfigure plugins (non-first parameters)',
+      async function () {
+        let calls = 0
+
+        unified()
+          .use(fn, givenOptions, givenOptions, givenOptions, undefined)
+          .use(fn, otherOptions, otherOptions, undefined, otherOptions)
+          .freeze()
+
+        assert.equal(calls, 1)
+
+        /**
+         * @param {unknown} a
+         * @param {unknown} b
+         * @param {unknown} c
+         * @param {unknown} d
+         */
+        function fn(a, b, c, d) {
+          assert.equal(arguments.length, 4)
+          assert.deepEqual(a, mergedOptions)
+          assert.deepEqual(b, otherOptions)
+          assert.deepEqual(c, undefined)
+          assert.deepEqual(d, otherOptions)
+          calls++
+        }
+      }
+    )
+
+    await t.test('should keep parameter length (#1)', async function () {
+      let calls = 0
+
+      unified().use(fn).use(fn).freeze()
+
+      assert.equal(calls, 1)
+
+      /**
+       * @param {...unknown} parameters
+       */
+      function fn(...parameters) {
+        assert.deepEqual(parameters, [])
+        calls++
+      }
+    })
+
+    await t.test('should keep parameter length (#2)', async function () {
+      let calls = 0
+
+      unified().use(fn, givenOptions).use(fn).freeze()
+
+      assert.equal(calls, 1)
+
+      /**
+       * @param {...unknown} parameters
+       */
+      function fn(...parameters) {
+        assert.deepEqual(parameters, [givenOptions])
+        calls++
+      }
+    })
+
+    await t.test('should keep parameter length (#3)', async function () {
+      let calls = 0
+
+      unified().use(fn).use(fn, givenOptions).freeze()
+
+      assert.equal(calls, 1)
+
+      /**
+       * @param {...unknown} parameters
+       */
+      function fn(...parameters) {
+        assert.deepEqual(parameters, [givenOptions])
+        calls++
+      }
+    })
+  })
 })