Skip to content

Improve plugin system#136

Merged
tinder-cfuller merged 6 commits intomainfrom
plugin-system
Jun 28, 2022
Merged

Improve plugin system#136
tinder-cfuller merged 6 commits intomainfrom
plugin-system

Conversation

@tinder-cfuller
Copy link
Contributor

@tinder-cfuller tinder-cfuller commented Jun 26, 2022

Removes Plugin Map while converting the data structure used by Plugin List from array to KeyValuePairs. This enables Plugin List to provide both ordered lookup as it does now as well as key based lookup that Plugin Map previously provided.

Plugin List also gains an overridable method for defining the order plugins are created in. This allows Plugin List to contain conditional logic (based on A|B test assignment or other dynamic configuration) similar to how Plugin contains logic within its isEnabled method. This new ability is crucial since Plugin List was significantly impaired by this missing capability until now.

Changelog

  • Remove Plugin Map
  • Use KeyValuePairs as Plugin List data structure
  • Add generic type to Plugin List for the Hashable key type
  • Add method to Plugin List to override plugin creation order
  • Add method to Plugin List to lookup plugins by key
  • Inject Plugin and Plugin List with component factories instead of component instances
  • Rename nested type-erased plugin type from Plugin to AnyPlugin
  • Update and improve source code documentation

Copy link
Contributor

@tinder-caiofonseca tinder-caiofonseca left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉

Copy link
Contributor

@tinder-garricnahapetian tinder-garricnahapetian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work! Please consider breaking up a PR like this into smaller ones in the future. Maybe something like this: 1) Refactor PluginList with creationOrder, 2) Remove PluginMap, 3) Rename to AnyPlugin 4) Update templates.

// swiftlint:disable:next strict_fileprivate
fileprivate func create(component: ComponentType, state: StateType) -> BuildType? {
let plugins: [KeyValuePair] = orderedPlugins(component: component)
for plugin: AnyPlugin in plugins.map(\.value).reversed() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does reversing still make sense with creationOrder? Without creationOrder, reversing made sense because the last plugin in the list was thought to be the latest experiment - for example. But as a user, if I define creation order, I would expect creationOrder to be respected. I would expect create (without key) to return the first plugin thats enabled in the creationOrder.

Copy link
Contributor Author

@tinder-cfuller tinder-cfuller Jun 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Creation order is being respected. The create method is meant to return the one that is created last. A mental model for this is that each one that is created overrides the previous one … imagine A is created, B is created, C is created and so C is returned.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that the implementation does not need to actually create A and B in the above example. By iterating in reverse order and returning the first non-nil one, we get the desired logic.

Copy link
Contributor

@tinder-caiofonseca tinder-caiofonseca left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👏

@tinder-cfuller tinder-cfuller merged commit 516c39f into main Jun 28, 2022
@tinder-cfuller tinder-cfuller deleted the plugin-system branch June 28, 2022 18:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

framework Framework

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants