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

refactor!: Separate ComponentSet from the Component #1266

Merged
merged 9 commits into from
Jan 14, 2022

Conversation

st-pasha
Copy link
Contributor

@st-pasha st-pasha commented Dec 22, 2021

Description

Currently, ComponentSet is tightly coupled with the Component class and is partially responsible for handling the Component's lifecycle.

This PR breaks the two classes apart: Component will now perform its own lifecycle management, and ComponentSet will focus on being a simple container.

Several optional breaking changes introduced here. "Optional" because they can be reverted, at the cost of making the code less nice. However, since ComponentSet is mostly internal class, I deem the risk to end users low:

  • Component.reAddChildren() was made private. Calling it by end users was dangerous.
  • Component.prepare() declared protected. Likewise, it was never intended to be user-callable.
  • ComponentSet no longer has field parent, as it is no longer needed.
  • ComponentSet constructor no longer has argument Component parent, same for ComponentSet.createDefault.
  • Since we're changing ComponentSet's constructor anyway, I use this opportunity to also change the comparator into a named argument, which allows it to be optional, so that no-argument ComponentSet() would create the component set with default comparator function. This also allows ComponentSet.createDefault to be greatly simplified, and in fact we will be able to remove it in Dart 2.15.
  • ComponentSet.addChildren() removed.
  • ComponentSet.add() and ComponentSet.addAll() are now prohibited (thus, the user can no longer say myComponent.children.add(newComponent)). In fact, it would be nice to prohibit direct manipulation of ComponentSet by the user altogether, but this PR doesn't go that far.

Out of scope for this PR: methods for removal of components.

Checklist

  • The title of my PR starts with a Conventional Commit prefix (fix:, feat:, docs: etc).
  • I have read the Contributor Guide and followed the process outlined for submitting PRs.
  • I have updated/added tests for ALL new/updated/fixed functionality.
  • I have updated/added relevant documentation in docs and added dartdoc comments with ///.
  • (NA) I have updated/added relevant examples in examples.

Breaking Change

  • Yes, this is a breaking change. (Indicate it in the Conventional Commit prefix with a !,
    e.g. feat!:, fix!:).

Copy link
Member

@spydon spydon left a comment

Choose a reason for hiding this comment

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

Lgtm!

);

// When we switch to Dart 2.15 this can be replaced with constructor tear-off
static ComponentSet createDefault() => ComponentSet();
Copy link
Member

Choose a reason for hiding this comment

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

Is this needed/used anywhere? The reason to have it before was bc it was actually configuring the created set. Now you could just call ComponentSet() directly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this is used by Component.childrenFactory. Somehow using a simple function comprehension () => ComponentSet() produces an analyzer warning there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Apparently, they fixed the warning just 8 days ago: dart-lang/linter#3118, but it will probably take time until we can actually use that.

@spydon spydon merged commit e2655b8 into flame-engine:main Jan 14, 2022
@st-pasha st-pasha deleted the ps/component-add-child branch January 14, 2022 18:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants