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

feat: add ParentIsA to force parent child relations #1566

Merged
merged 10 commits into from
Apr 27, 2022
Merged

Conversation

wolfenrain
Copy link
Contributor

Description

The HasParent mixin allows the developer to ensure that their component will always have a given parent type. It can't be added to a parent of a different type.

Therefore it can provide a strongly typed parent.

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 ///.
  • I have updated/added relevant examples in examples.

Breaking Change

  • Yes, this is a breaking change.
  • No, this is not a breaking change.

Related Issues

Copy link
Member

@erickzanardo erickzanardo left a comment

Choose a reason for hiding this comment

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

LGTM, like we discussed, the only thing I am not sure about is the name, we should make a more strong statement with this mixin name since it is used to do some validation, some ideas:

  • EnsuredParent
  • ChildOf
  • BelongsTo
  • RequiredParent

This is a hard name to find, so if we can't find any alternative, I am fine with HasParent as well

@st-pasha
Copy link
Contributor

Possible names:

  • ParentIsA<T>
  • ParentHasType<T>

The closest equivalent in Dart is the isA<T>() matcher in flutter_test.

@wolfenrain
Copy link
Contributor Author

wolfenrain commented Apr 23, 2022

Possible names:

  • ParentIsA<T>
  • ParentHasType<T>

The closest equivalent in Dart is the isA<T>() matcher in flutter_test.

The ParentIsA<T> is best suited in my opinion! It explains it and tells the developer that it will enforce something as well.

@alestiago
Copy link
Contributor

alestiago commented Apr 23, 2022

I fully agree with @wolfenrain, my vote goes for ParentIsA<T>. I love it! Great suggestion @st-pasha!

My only concern is how it deviates from the standard HasFoo we have going on.

@@ -158,6 +158,24 @@ available eventually: after they are loaded and mounted. We can only assure
that they will appear in the children list in the same order as they were
scheduled for addition.

#### Ensuring a component has a given parent
Copy link
Contributor

Choose a reason for hiding this comment

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

This should probably be a 3rd-level header (###), just like all other sections in here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did that as a fourth as I didnt find a good place to put this, and this chapter talks about children order and parent relations so I thought this might be a nice fit.

Copy link
Member

Choose a reason for hiding this comment

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

I think this would be fine as a third level header too, if you look at the section after this one it is also third level and I think they fit on the same level.

doc/flame/components.md Show resolved Hide resolved
doc/flame/components.md Outdated Show resolved Hide resolved
packages/flame/test/game/mixins/has_parent_test.dart Outdated Show resolved Hide resolved
await parent.add(component);
await game.add(parent);

expect(component.parent, isA<ParentComponent>());
Copy link
Contributor

Choose a reason for hiding this comment

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

This would have succeeded even if there was no HasParent mixin.
Perhaps we could add some ParentComponent { int foo; } property, and then check that component.parent.foo can be read without an explicit type-cast?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem with that is, we can't. It would fail before that, and if it didn't fail it will be correct anyway. It wouldn't test anything really. This whole test doesn't really test anything except that there was no error from the add

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.

ParentIsA sounds good to me too!

@st-pasha
Copy link
Contributor

I'm worried about overriding addToParent though -- perhaps the previous approach with onMount is better?

This is because there are several ways to add a component to a given parent, and addToParent is only one of them. There is also add() and addAll() which currently defer to addToParent, but this is merely an implementation detail which may change at some point in the future. Then, there is changeParent() which is currently independent of addToParent, which means it would circumvent the check in ParentIsA<T>.

@wolfenrain
Copy link
Contributor Author

I'm worried about overriding addToParent though -- perhaps the previous approach with onMount is better?

This is because there are several ways to add a component to a given parent, and addToParent is only one of them. There is also add() and addAll() which currently defer to addToParent, but this is merely an implementation detail which may change at some point in the future. Then, there is changeParent() which is currently independent of addToParent, which means it would circumvent the check in ParentIsA<T>.

You bring up a good point here, it is indeed more susceptible to breaking changes (even tho we should catch these bugs). But the onMount has the downside that the user will only know it was "wrong" after the component gets mounted and not when you are already adding it to a parent.

I personally think that the onMount is indeed a safer bet to guarantee that it won't break when we rework our adding logic.

@wolfenrain wolfenrain requested a review from spydon April 26, 2022 18:35
@wolfenrain wolfenrain changed the title feat: add HasParent to force parent child relations feat: add ParentIsA to force parent child relations Apr 26, 2022
@spydon spydon enabled auto-merge (squash) April 27, 2022 17:40
@spydon spydon merged commit 2cdf386 into main Apr 27, 2022
@spydon spydon deleted the feat/add-has-parent branch April 27, 2022 17:47
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.

5 participants