feat: add derive to FASTElementDefinition for overriding existing FAST element definition properties

[KingOfTac]

Pull Request

📖 Description

This PR is a proposal to split the name property on FASTElementDefinition in a prefix and baseName. While building multiple web component libraries that build on top of fast-element and fast-foundation I have noticed cases where library consumers sometimes need to change the element prefix after a definition has already been composed. This adds a prefix property that can be changed and a readonly baseName property. The name is now a readonly getter that combines the prefix and baseName.

edit: Based on feedback, this PR now adds a derive method to FASTElementDefinition which uses the compose API under the hood. This enables consumers of libraries that export definitions to still override and customize the element definitions before defining them with the platform.

🎫 Issues

👩‍💻 Reviewer Notes

📑 Test Plan

Tests continue to pass. This shouldn't be a breaking change due to the name getter still returning the same result as before and the arguments passed to FASTElementDefinition have not changed.

Added tests for the new derive method. Existing tests continue to pass.

✅ Checklist

General

• [X] I have included a change request file using $ yarn change
• [x] I have added tests for my changes.
• [X] I have tested my changes.
• [X] I have updated the project documentation to reflect my changes.
• [X] I have read the CONTRIBUTING documentation and followed the standards for this project.

Component-specific

• [ ] I have added a new component
• [ ] I have modified an existing component
• [ ] I have updated the definition file
• [ ] I have updated the configuration file

⏭ Next Steps

[EisenbergEffect]

There could be another API that freezes the definition, removing those APIs and locking things down.

[chrisdholt]

There could be another API that freezes the definition, removing those APIs and locking things down.

I'd love to see this - @kingoftac, I'm fine with a singular change to override "name" here in isolation, but would prefer the larger API overrides to include a method to prevent or freeze as @EisenbergEffect points out.

[KingOfTac]

@chrisdholt @EisenbergEffect hmm. I wonder if the solution here would be add an additional property to the definition to "freeze" it at the point it is created initially. Internally this could return a definition that omits the derive method entirely. Thoughts?

[KingOfTac]

I added a freeze method that returns a frozen instance.

I'm curious what everyone's thoughts are on the change to derive from an instance method to a property. Is omitting derive from the type signature sufficient on its own or do we want to remove derive entirely from the instance?

Changing derive to a property could also suggest to authors that it can be overridden which may be undesirable, at least with the current type signature of Function | undefined. We would likely need to use a stronger type.

One thing to note while reviewing with these questions in mind is the ergonomics of using derive with these changes. Derive would now need to be used with an optional chaining operator: myDefinition.derive?.('new-name')

edit: an example of these new changes

// simple derive use case

// foo-library/button.js
export const fooButton = FASTButton.compose(config);

// my-library/button.js
// overrides name and styles and re-exports
import { fooButton } from 'foo-library';

export const myButton = fooButton.derive?.({
  name: 'my-button',
  styles: myButtonStyles
})

// preventing derived instances from being created with freeze

// foo-library/button.js
export const fooButton = FASTButton.compose(config).freeze();

// my-library/button.js
// overrides name and styles and re-exports
import { fooButton } from 'foo-library';

// myButton will be undefined
export const myButton = fooButton.derive?.({
  name: 'my-button',
  styles: myButtonStyles
})

[nicholasrice]

I think requiring an undefined check to call derive isn't a good dev experience. I'd prefer to see a simple massaging of types and deletion of the property or create the frozen class implementation w/o the freeze and derive methods, extend that class to make FASTElementDefinition that adds the freeze and derive methods, and then freeze() would return a new instance of the base class.

I do wonder if it really needs a freeze() method to remove the derive method though. Manually constructing a new definition from another definition is pretty easy, so freezing doesn't really prevent anything, it just makes it a little more tedious.

[KingOfTac]

Circling back to this. @EisenbergEffect @chrisdholt any thoughts/suggestions regarding @nicholasrice's comment? It would seem there are some different ideas in how the API should work so I'd like to get that nailed down before I make any other changes. I don't really have any strong opinions on the implementation details, other than thinking being able to derive new definitions from existing ones would be a useful QoL feature.

[chrisdholt]

Circling back to this. @EisenbergEffect @chrisdholt any thoughts/suggestions regarding @nicholasrice's comment? It would seem there are some different ideas in how the API should work so I'd like to get that nailed down before I make any other changes. I don't really have any strong opinions on the implementation details, other than thinking being able to derive new definitions from existing ones would be a useful QoL feature.

I think we started w/ deriving a name and it broadened to additional API's for overriding all the things. If we look beyond our own scopes for libraries and just at an author creating a WC, making it tedious for someone to change those things is sometimes by design as the intent of the component is that it sometimes should not be modified. I also read @nicholasrice's statement a bit inverted, "Manually constructing a new definition from another definition is pretty easy", especially if you have all the parts. If the name is the problem here, let's focus on deriving a new name - although I think the same argument can be made...does an author want to make that easy?

If manually constructing these isn't desired, I'd ask if perhaps having an ExtensibleFASTElementDefinition as a class which supports this derive behavior is a better alternative. A separate method of constructing for extensibility means we as a library aren't forcing authors into supporting a behavior they may not want.

[nicholasrice]

Definitions don't support changes because all of the defining properties are readonly, so we're talking about creating new definitions and whether that is easy (derive()) or hard (manually copying the properties into a new .compose() call). If a library exposes the definition, then new definitions can be derived from it with or without .derive(), which is why I don't see a lot of utility in .freeze(). If we want authors to be able to lock down definitions, then we need to not expose all of the defining properties (templates, styles, etc) from the definition object, which is obviously a larger change.

[chrisdholt]

Definitions don't support changes because all of the defining properties are readonly, so we're talking about creating new definitions and whether that is easy (derive()) or hard (manually copying the properties into a new .compose() call). If a library exposes the definition, then new definitions can be derived from it with or without .derive(), which is why I don't see a lot of utility in .freeze(). If we want authors to be able to lock down definitions, then we need to not expose all of the defining properties (templates, styles, etc) from the definition object, which is obviously a larger change.

@nicholasrice I guess my primary point is, should we make it easy to change everything by default as this change proposes? I'm not sure. I'd probably prefer an RFC if we're going to add more than name by default to get feedback from the community.

[KingOfTac]

@chrisdholt @EisenbergEffect @nicholasrice I'm going to close out this PR and open an RFC for the API changes.