Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[heft-config-file] Add inline inheritanceType specification in configuration files #3276

Merged
merged 8 commits into from
Mar 19, 2022

Conversation

D4N14L
Copy link
Member

@D4N14L D4N14L commented Mar 14, 2022

Summary

Implements inline inheritanceType properties, largely following the spec from #3181.

Details

When extending a base configuration file, this feature allows a developer to specify $<targetProperty>.inheritanceType properties to customize how the specified property will be inherited into the configuration file. Previously, this was only possible at the configuration file class level, and only for top-level properties. Additionally, keyed object types are now able to be merged.

Currently, support is provided for the following inheritanceType values:

  • replace
  • append (Only Array properties)
  • merge (Only keyed object properties)

For example, if you have a file configB.json that extends configA.json, the following can now be accomplished:

configA.json:

{
  "propA": [ 1, 2, 3 ],
  "propB": {
    "propC": {
      "propD": "From configA"
      "propE": [ "From configA" ]
      "propF": "Also from configA"
    }
  }
}

configB.json:

{
  "propA": [ 4, 5, 6 ],
  "$propB.inheritanceType": "merge",
  "propB": {
    "$propC.inheritanceType": "merge",
    "propC": {
      "propD": "From configB",
      "$propE.inheritanceType": "append",
      "propE": [ "From configB" ]
    }
  }
}

Result:

{
  "propA": [ 1, 2, 3, 4, 5, 6 ],
  "propB": {
    "propC": {
      "propD": "From configB"
      "propE": [ "From configA", "From configB" ],
      "propF": "Also from configA"
  }
}

By default, the same behavior as the existing @rushstack/heft-config-file will be used for inheritance at all levels, making this change backwards compatible. The current behavior is:

  • Arrays: append
  • All other types: replace

Strictly speaking, this makes explicitly specifying the append value redundant in cases where top-level arrays aren't set to replace by the configuration file class, since the only other inheritance type available on arrays would be to replace. However, it is included for clarity during configuration file specification. In a somewhat similar manner, specifying replace on an object would be equally redundant, however the replace value would still be required for Arrays.

One notable change to existing behavior is that schema validation is now done only after the configuration object is fully constructed through the extends-stack. Previously, each configuration file would be individually validated against the schema. This would have made it impossible to inherit values from a previous object while modifying other existing values.

How it was tested

Added multiple test cases to validate inheritance behavior in a variety of scenarios, including handling of error cases.

@iclanton
Copy link
Member

What if we said the magic properties are objects, like:

{
  "$propName": {
    "inheritaneType": "merge",
    // other options we add someday
  },
  "propName": ...
}

@D4N14L
Copy link
Member Author

D4N14L commented Mar 16, 2022

What if we said the magic properties are objects, like:

{
  "$propName": {
    "inheritaneType": "merge",
    // other options we add someday
  },
  "propName": ...
}

Given the limited additional properties we would be adding here, I feel this would be better handled by stacking the properties, so something like:

{
  "$propName.inheritanceType": "merge",
  "$propName.otherOption": "...",
  "propName": "..."
}

While yours is absolutely more flexible, I'm not sure that the added flexibility trumps the conciseness of the proposed format.

Or, I guess, we could support both styles, such that the simplified $propName.<option> maps to an object with an <option> property internally? So that the two formats are functionally equivalent?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants