[request] wpjson.settings callback fn required to return passed theme instance; missing generated theme.json parts otherwise

[strarsis]

Agreement

• This is not a duplicate of an existing issue
• I have read the guidelines for Contributing to Roots Projects
• This is not a personal support request that should be posted on the Roots Discourse community

Describe the issue

The wpjson.settings callback function has to return the theme instance which it got passed as argument,
as otherwise parts of generated theme.json will be missing, and not just the parts added after the callback function.

Expected Behavior

No missing parts of generated theme.json, even if the callback function does not return the theme instance.

Actual Behavior

Parts of theme.json is missing when the callback function does not return the theme instance:

Steps To Reproduce

1. Start with a default Sage theme with default bud configuration.
2. Run the bud build. (Optional: Keep/copy generated theme.json for later comparison with a newly generated theme.json).
3. Add to bud config a wpjson.settings invocation with a callback function that does not return the theme instance that was passed to it as argument:

// [...]
      .settings(theme => {
        // logic that does not return `theme`
       // return theme; // do not return `theme` instance to reproduce this issue
      })
// [...]

4. Run the bud build, note that parts of newly generated theme.json are missing now.

version

6.9.1

Logs

No notices, warnings or errors were logged.

Configuration

Default `bud` config file, except for the added `wpjson.settings` for reproducing this issue.

Relevant .budfiles

No response

[kellymears]

I'm willing to hear out arguments for the behavior being different, but this function is working as intended (and as documented).

It gives you the object and asks you to return something:

export interface Mutator {
  (
    json:
      | Partial<Theme.GlobalSettingsAndStyles['settings']>
      | Container<Partial<Theme.GlobalSettingsAndStyles['settings']>>,
  ):
    | Partial<Theme.GlobalSettingsAndStyles['settings']>
    | Container<Partial<Theme.GlobalSettingsAndStyles['settings']>>
}

interface settings {
  (
    input?:
      | Mutator
      | Container<Partial<Theme.GlobalSettingsAndStyles['settings']>>
      | Partial<Theme.GlobalSettingsAndStyles['settings']>
      | boolean,
    raw?: boolean,
  ): this
}

I think it's more confusing for this function to not use what is returned as a value. And it's pretty easy to always return a value with arrow fns and the container interface:

bud.wpjson.settings(theme => theme
  .set('color.custom', false)
  .set('color.customDuotone', false)
)

Regardless, wpjson.settings should arguably be deprecated in #2079 (currently not planning on that). But, I would use bud.wpjson.set over bud.wpjson.settings (PR soon on sage itself):

bud.wpjson
  .set(`settings.color.custom`, false)
  .set(`settings.color.customDuotone`, false)
  .set(`settings.color.customGradient`, false)
  .set(`settings.color.defaultDuotone`, false)

[github-actions]

Message to comment on stale issues. If none provided, will not mark issues stale