fix: Component can now be removed in any lifecycle stage

[st-pasha]

Description

• It should now be possible to remove a component from any possible lifecycle stage.
• The Component._state variable converted from an enum into a bitfield in order to be able to better track the state.
• Removed restriction that components can't be added to a game from the constructor.

Checklist

• The title of my PR starts with a Conventional Commit prefix.
• 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 dartdoc comments.
• [-] 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

Closes #1549
Closes #1532

[spydon]

Looks good! Checks are failing but I'm sure you're aware.

[alestiago]

Nice work!

I still need to have a better dive. But overall I'm impressed on the quality of the documentation.

Having said this, I'm concerned about using bit operations. My concern is that usually Flutter developers do not perform these type of operations, hence this code can be seen as slightly more difficult to comprehend.

In addition, personally I think that it requires more thought to read bit operations since I calculate them mentally, over just reading if something is equal to something else.

I'm thinking how can we make this code more readable, especially for beginners.

Perhaps we can have an extension on int and have methods like has(int value) that checks that this && value != 0 (mask). It would be nice to keep the performance whilst not sacrificing so much readability. Or keep the enum but have more states and map them to bits with an extension (until enhanced enums) or define methods that do the checks you are interested.

Also, I feel that there are many combinations that _state can have. With no guarantee that an invalid state can be reached. Making it more error prone and difficult to maintain. Perhaps we can have a setter that, for example, checks that the state is not loaded and removed simultaneously. A possible implementation is to have the but assertion that checks _state is always a valid state.

Finally, I needed a removed completer. Do you think this makes it easier to implement this?

[spydon]

Having said this, I'm concerned about using bit operations. My concern is that usually Flutter developers do not perform these type of operations, hence this code can be seen as slightly more difficult to comprehend.

They are only internal, I think this is a perfect use for bitfield and bit operations.
(And if someone wants to do changes to this behavior, it really isn't any rocket science to read up on how it works)

I'm thinking how can we make this code more readable, especially for beginners.

Beginners really shouldn't be fiddling around in this part of the code. 😅

Also, I feel that there are many combinations that _state can have. With no guarantee that an invalid state can be reached. Making it more error prone and difficult to maintain. Perhaps we can have a setter that, for example, checks that the state is not loaded and removed simultaneously. A possible implementation is to have the but assertion that checks _state is always a valid state.

That's a good point, but I think there are sufficient tests (and if there aren't they should be added) that ensures that a faulty state can't be reached, note again that the user isn't manipulating this field freely, but the internal state machine changes it.

Finally, I needed a removed completer. Do you think this makes it easier to implement this?

I don't see any removed completer, which one was it?

[erickzanardo]

I must confess that bit operations scares me a little bit (HA, get it?).

Not that I can't understand them, but they are not so human friendly, so if possible I think we could add some internal comments on the lines where the operations take place.

But even with that said, this is a nice clean up, so LGTM!!

[st-pasha]

Thanks for the feedback, I see there's some desire for more comments about how this all fits together, so i'll try to add those later tonight.

[alestiago]

@spydon I don't need any completer to be removed. Sorry for the confusion I meant currently we have a "loaded" completer, a "mounted" completer, but we don't have a "removed" completer.

I was wondering if after this change we could add it.

[st-pasha]

I was wondering if after this change we could add it.

We can, absolutely

[spydon]

Excellent docs for _state, probably the longest one we have for a field anywhere in the whole code base.

[spydon]

Couldn't we place it straight into the loading list if it is only halfway through loading when it is re-added?

[st-pasha]

I'm worried about the case when the user is accessing parent from onLoad, and then suddenly that parent would change or become null in the middle of their code.
Of course, accessing parent from onLoad is a kind of an anti-pattern, but we have been allowing it so far, so not sure how to handle this.

We could, for example, make it an error to access parent while the component isLoading -- but that would be a breaking change, and has to be done in a separate PR, after careful consideration.

[spydon]

Right, I think that could be a good change so that people move such logic to onMount instead.
But as you said, outside of the scope of this PR then.