framework icon indicating copy to clipboard operation
framework copied to clipboard

suggestions for layered config

Open Fil opened this issue 1 year ago • 2 comments

Merging the result of the function with its argument feels surprising to me. I'm suggesting this alternative, where we only take the result of the function (the function can do {...config, title: "new title"} if that is what the user wants).

Also, I'm suggesting to await, so the exported function could be async? (Or, we wait for a real-world use case before we do that?)

Fil avatar May 24 '24 07:05 Fil

I like making the function async, that's a good idea. Also, thanks for the documentation improvements.

I'm not convinced about not merging the config. I actually had it this way originally, and I changed it to merge after working with it a bit. My thinking was that when you use a config file that exports an object, you are essentially providing a patch for the configuration. With the merging version, the function has the exact same semantics: it produces a patch.

In other words, I think these two configuration files should do the same thing, because wrapping the config in a function shouldn't change the outcome.

export default { title: "Override" }
export default () => ({ title: "Override" });

What do you think? Is that a compelling reason to keep the merging behavior? In the end I'm more interested in being able to layer configs than I am in the specific implementation, so I can go with either.

Also, I suspect the version here will fail the unit tests, since there is a test for the old behavior. We could update that in the main branch though if needed.

mythmon avatar May 24 '24 15:05 mythmon

I think the result being merged into the input is a surprising API, but I hear you on the consistency and convenience. In both cases we can get the same results, so it's not blocking. Maybe ignore this part of suggestion?

Fil avatar May 24 '24 17:05 Fil

superseded by #1396

Fil avatar May 30 '24 07:05 Fil