feat!: make all settings mergeable

[jj811208]

What's the problem this PR addresses?

related: #3591

Make all settings mergeable, and provide a reset: true syntax when they need to be a full override

closes #4341
closes #2106

...
How did you fix it?

• declare a new function to get the existing value of field in the configuration tree 4da8346

/*
example:
configuration.values = {
  a: {
    b: [{ c: 2 }],
  },
};
*/
getCurrentValue(`a.b.0.c`) // 2

• define a new parameter reset for useWithSource

• I put the configuration.use opts into the parseXxx family of functions so that the merge and overwrite and reset actions work throughout the configuration tree

• by the above three points, make shape, map, array mergeable in depth 0f9a52e

• I added some tests to verify that I didn't break anything f816430

current behavior

opts.overwrite

if true, overwrite currentValue with the new calculated value

// 1. single value (all types except shapes, maps, and arrays)
return (overwrite || currentValue === undefined)? newValue : currentValue

// 2. shape, map
return new Map(overwrite ? [...currentValue, ...newValue] : [...newValue, ...currentValue]);

// 3. array
return (definition.concatenateValues && !overwrite) ? [...newValue, ...currentValue] : [...currentValue, ...newValue];

opts.reset

if true, currentValue will always be undefined | empty map | empty array

const currentValue = reset ? undefined : getCurrentValue(configuration, path);

...

Checklist

• I have read the Contributing Guide.

• I have set the packages that need to be released for my changes to be effective.

• I will check that all automated PR checks pass before the PR gets reviewed.

[RDIL]

cc @paul-soporan in case this overlaps with the work you are doing related to config I think

[arcanis]

I think I haven't been clear enough on the original issue, sorry 😅

When I said having a reset: true parameter, I meant inside the Yaml itself. For instance (I'm not fixed on the syntax; in these examples I assume that wrapping an object or array inside an object with a single reset property will prevent the wrapped object from being merged, and extend would override it).

{"foo": {"hello": true}} + {"foo": {"world": true}}
  == {"foo": {"hello": true, "world": true}}

{"foo": {"hello": true}} + {"foo": {"reset": {"world": true}}}
  == {"foo": {"world": true}}

{"foo": {"hello": true}} + {"foo": {"world": true}, "bar": 42}
  == {"foo": {"hello": true, "world": true}, "bar": 42}

{"foo": {"hello": true}, "bar": 42} + {"reset": {"foo": {"world": true}, "baz": 21}}
  == {"foo": {"world": true}, "baz": 21}

{"foo": {"hello": true}} + {"reset": {"foo": {"extend": {"world": true}}, "bar": 42}}
  == {"foo": {"hello": true, "world": true}, "bar": 42}

It'd work the same for arrays:

["hello"] + ["world"]
  == ["hello", "world"]

["hello"] + {"reset": ["world"]}
  == ["world"]

I'd assume that array contents aren't merged, since I don't see any way to do it in a clean way:

[{"foo": 21}] + [{"bar": 42}]
  == [{"foo": 21}, {"bar": 42}]
  != [{"foo": 21, "bar": 42}]

[{"foo": 21}] + {"reset": [{"extend": {"bar": 42}}]}
  == [{"bar": 42}]
  != [{"foo": 21, "bar": 42}]

This way, we wouldn't have to change the public API at all, and users would be able to define whether they want to override settings straight from their setting file.

Does that make sense?

[jj811208]

When I said having a reset: true parameter, I meant inside the Yaml itself. For instance (I'm not fixed on the syntax; in these examples I assume that wrapping an object or array inside an object with a single reset property will prevent the wrapped object from being merged, and extend would override it).

I've thought about it, but I'm worried I'm going in the wrong direction 🙈.

I think all of yarn settings can be [1]

type Value = unknown; // Depends on `SettingsType`;

interface ConfigurationValue = Value | {
  value: Value; 
  strategy: 'extend' | 'reset';  // default is `extend`
}

this is similar to the current plugin definition (String | Shape) and does not conflict with the new parser.

I'd assume that array contents aren't merged, since I don't see any way to do it in a clean way:

I think so too, this PR currently works like this 🚀

This way, we wouldn't have to change the public API at all, and users would be able to define whether they want to override settings straight from their setting file.

I can remove the reset option from configuration.use and configuration.useWithSource (as it was originally), but keep the parseXxx option 🤔 .[2]

Does that make sense?

yes! I think so.

---

if [1] + [2] is OK, that way I think the changes requested in this comment should be easy.

[jj811208]

@arcanis can you take a look at this new test? It shows the way my last comment said 🥸

1081a77

but this will make strategy a yarn's reserved word

---

I may also reverse the order in which rcfiles are read, currently it reads from child directory to parent directory,

however, this will result in the parent directory's rcfile having more weight when multiple rcfiles have a "reset" strategy on the same field.

[arcanis]

nit: not a fan of strategy: reset, the "strategy" here doesn't look super clear to me ... onConflict: reset, maybe? 🤔

And do we need the value field? Perhaps we could just use the remaining keys as the proper object; it would allow to remove an indentation level if someone wanted to use onConflict: reset at the top-level to disable the yarnrc inheritance altogether:

onConflict: reset

npmRegistryServer:
  # ...

[jj811208]

And do we need the value field? Perhaps we could just use the remaining keys as the proper object

I think your method will make the configuration easier, but I'm not sure how this works in ARRAY and SINGLEVALUE, would you be willing to tell me more 🙏?

onConflict: "xxx"

// MAP & SHAPE
packageExtensions:
  onConflict: "xxx"
  gatsby-cli@*:
    onConflict: "xxx"
    peerDependencies:
      onConflict: "xxx"
      eslint: "*"
      gatsby: "*"
      
// ARRAY
🤔

// SINGLE VALUE (string, number, boolean)
🤔
/* 
(Currently, if a single value has the `reset' option, it will still overwrite the original value,
even if `useWithSource.opts.overwrite === false`, but this may be an overdesign 🧐.)
*/

[arcanis]

Yes, in the case of array we'd probably need to have a value field, I don't see a clear way to avoid it there... 🤔

It's a first iteration anyway, we can improve that later on if we find a prettier syntax

(Currently, if a single value has the reset option, it will still overwrite the original value, even if useWithSource.opts.overwrite === false, but this may be an overdesign 🧐.)

Looking at its usage, we probably should remove useWithSource.opts.overwrite entirely, and replace it by adding onConflict: reset to all the places that used it.

[jj811208]

These days I have done some things

1. reverse the read order of rcfile 7dae8d7

2. I removed useWithSource.opts.overwrite and BaseSettingsDefinition.concatenateValues (It doesn't seem to matter much, and it doesn't seem to work without overwrite 🧐)

3. I implemented onConflict and it now has three behaviors

   extend(default)

   attempt to merge or overwrite(reset)

   reset

   overwrite a field whether it has a value or not

   skip

   if the value already exists in the field, it will not be parsed

c394b7f
c83c0f7

4. I modified the test to cover the onConflict behavior
   948c5be

---

updated:
currently the plugins field is not supported using onConflict, because its process is different from other field.

[jj811208]

I fixed it and now ready for review 🤡 adc7ff0

[arcanis]

I made a couple of changes:

• I noticed that {"onConflict": "reset"} at the top-level wasn't resetting values that weren't in the file, so I tweaked the code to strip the overrode configuration settings from all yarnrc before the one with onConflict: reset
• I also noticed that {"onConflict": "reset", "immutablePatterns": ["foo"]} was extending immutablePatterns rather than overriding it (I think because the onConflict wasn't extracted as part of the core fields). Fixed that.
• The environment wasn't prioritized over the yarnrc files. Fixed that.

Generally I'm a little worried of the implementation complexity, and especially that it's interlaced with the rest of the configuration parser. If the tests pass we can land it as-is, but I think it could make sense to potentially revisit this part later and extract the merging logic as a separate pass, before the settings are interpreted. In pseudo-code, something like:

const rcFiles = getAllRcFiles();
const rcFilesWithoutOnConflict = resolveOnConflictFields(rcFiles);
configuration.use(rcFilesWithoutOnConflict);

This way the resolveOnConflictFields logic would be entirely separate, which I suspect could make it much easier to test and reason about 🤔

In any case, let's see how the tests go!

[arcanis]

I fixed a test, but a failing one remains - my understanding is that the code here is problematic:

berry/packages/yarnpkg-core/sources/Configuration.ts

Lines 685 to 694 in 32772b5

	if (value === undefined) {
	// XXX: `enableStrictSettings` is a very special setting, it should only be used by default when importSettings
	if (path === `enableStrictSettings`)
	return undefined;
	if (reset || current === undefined)
	return defaultValue;
	return current;
	}

That's because it makes the configuration think that yarnPath is set by <environment> (because it has an undefined yarnPath value, which gets turned into whatever current is), which messes with the --only-if-needed flag in yarn set config. It wasn't a problem in your commits because the environment had a lower priority than the yarnrc files (which was a bug).

If you can take a look (either to hotfix this, or to look at changing the implementation to be closer from a preprocessor, as I suggested in the previous post) it'd be great!

[jj811208]

No problem ~ there is still enough time before the official release of Yarn 4, so I don't mind spending more time if I can make it better 😀.

[jj811208]

1. I reverted most of the previous changes 🙈, now configuration.use, configuration.useWithSource, parseXxx, overWrite, concatenateValues are almost the same as before.

2. we originally needed to pass in cwd to parseXxx so that the ABSOLUTE_PATH type could also calculate the relative path, but now everything is inside valueBase.
   In order to not make a breaking change to use() in this PR, I have declared a suspicious looking variable here 💩
   https://github.com/yarnpkg/berry/pull/4982/files#diff-fc305739a8974ac3d1d60a845b2eb65a0b55dd5e87085c2a5a4232ba276b2b47R1034-R1035

3. thank @arcanis for the review and sample code, it really helps a lot 😀!

[arcanis]

Looks generally good to me, although I'll wait for @RDIL to make a review pass as well since he asked to do it - two nice potential follow ups (for subsequent PRs, I think it's fine to land this one as-is and improve incrementally):

• Rework the RESOLVED_RC_FILE marker so that it isn't needed
• Make Configuration#use accept onConflict markers

[arcanis]

I'm not a fan of this pattern - I'd prefer if the manipulated were always of a consistent type, even if it requires normalizing them beforehand.

[RDIL]

Thank you @jj811208, this is great!