feat: Added HasGameReference mixin

[st-pasha]

Description

This almost exactly like the current HasGameRef mixin, except that:

• The property is called game instead of gameRef (the "gameRef" violates Dart naming conventions against using abbreviations in variable names);
• The template type T supports all Games, not only FlameGames;
• Better integration with the SingleGameInstance mixin.

Checklist

• [x] The title of my PR starts with a Conventional Commit prefix (fix:, feat:, docs: etc).
• [x] I have read the Contributor Guide and followed the process outlined for submitting PRs.
• [x] I have updated/added tests for ALL new/updated/fixed functionality.
• [x] 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.
• [x] No, this is not a breaking change.

Related Issues

[st-pasha]

@erickzanardo Shouldn't we take a more careful approach here: instead of merging the two classes, merely mark the old one as deprecated? That way the users will be able to switch at their own pace, instead of being forced into it.

[erickzanardo]

@st-pasha from what I see on your implementation, despite the name changes, everything could be accommodated on the current HasGameRef implementation.

The generics would change, but a FlameGame is a Game, so there wouldn't be a breaking change here. And how you made the retrievement of the game instance is an internal thing, just like on the HasGameRef, so that could go in there too.

I know that the naming convention may be off, but I would rather keep it instead of adding a big (big because this is widely used) just because of renaming, and avoid frutastions like this.

[st-pasha]

Ok, I've merged the two implementations then: the old name is marked as "Deprecated", but only in comments (so everything continues to work as before). We could upgrade the comment to "real" deprecate-annotation, either in this PR or at some later time (perhaps after the next release).

[renancaraujo]

Sorry for popping in so late into the discussion, but I seriously think that we should slow down on breaking core API. I agree that HasGameReference is better than HasGameRef and a game property makes more sense than gameRef, but, renaming the core API has some serious trade-offs for example: making existing tutorial blogs and videos totally outdated, let alone all the projects people have already deployed

In my opinion, the juice doesn't worth the squeeze here. The rule of thumb on changing core API should be "we rename it only if it becomes misleading or sufficiently inaccurate".

All the other aspects of this PR are great and I appreciate all the work done here ❤️

Edit:

Deprecation is just a less harsh way of publishing a breaking change. I wouldn't rename it at all.

[st-pasha]

@renancaraujo I agree completely, breaking changes should be avoided at all costs, and when necessary introduced at a very slow pace. Which is why in the first submission I had the HasGameReference mixin completely separate from HasGameRef. This way we can transition from the old mixin to the new one reaaaally slow:

• at first start mentioning in Discord that we prefer HasGameReference mixin now;
• change our documentation/examples to use the new mixin only;
• add a note somewhere that HasGameRef will eventually be deprecated;
• deprecate it only at some future date, say in 6 months or in a year;
• finally remove after it was deprecated for some long time.

This way we could still introduce code improvements, but with no disruption to the users.

The only con of this approach is that it results in some code duplication -- but I don't see it as that much of a big deal. Code duplication when migrating between different APIs is quite common, and doesn't result in increased maintenance cost.

Should I revert to that original version of the PR?

[spydon]

I agree with @renancaraujo and @erickzanardo, it seems unnecessary to rename the mixin and gameRef -> game just because it breaks the naming convention. The PR overall is good though, can't the HasGameRef just be changed to have the behaviour of HasGameReference, without changing any naming?

[st-pasha]

Alright, I've restored HasGameRef to the way it originally was. Now:

1. No changes for existing users whatsoever (not even deprecation);
2. If anyone gets annoyed by less-than-perfect grammar (like me), they can switch to HasGameReference and .game.

Thus, this PR is a net-positive for some users, and neutral for everyone else.

[spydon]

Alright, I've restored HasGameRef to the way it originally was. Now:

1. No changes for existing users whatsoever (not even deprecation);

2. If anyone gets annoyed by less-than-perfect grammar (like me), they can switch to `HasGameReference` and `.game`.

Thus, this PR is a net-positive for some users, and neutral for everyone else.

I don't think that we should keep HasGameReference, it will just lead to confusion having two mixins doing the same thing.

[st-pasha]

it will just lead to confusion having two mixins doing the same thing.

But that is exactly how code evolution works, if we want to avoid breaking changes.

If there is a class Foo whose behavior/API needs to be changed in a way that would be breaking to the users if the change was made directly, then the solution is to create class Foo2 with the new API, then recommend the users to switch to Foo2, and deprecate Foo, then after some period of time (i.e. several releases) the old class Foo can be removed.

While both Foo and Foo2 coexist, they'll be doing the same thing.

The confusion can be remedied by just having a simple comment explaining that one class is an old version and the other is a new version.

[spydon]

But that is exactly how code evolution works, if we want to avoid breaking changes.

This is not a necessary evolution though and we can avoid it. The code changes can be done in HasGameRef and the name change is not more important than avoiding a breaking change even though it would be a slow transition, the whole team has agreed on this.

[st-pasha]

This is not a necessary evolution though and we can avoid it. The code changes can be done in HasGameRef and the name change is not more important than avoiding a breaking change even though it would be a slow transition

So, what you're saying is that we want to avoid even a slow deprecation cycle here?

It's fine by me -- in fact, the PR is already structured that way. We could have the two mixins live side-by-side, giving the users an opportunity to choose which one they prefer. Perhaps I could add some more clarification in doc-comments about this, so as to avoid any potential confusion on the users part.

[spydon]

So, what you're saying is that we want to avoid even a slow deprecation cycle here?

No, what I'm saying is that the HasGameReference mixin shouldn't be introduced at all. The changes done to the code in HasGameReference should be possible to add in a non-breaking way to HasGameRef, right?

[st-pasha]

No, what I'm saying is that the HasGameReference mixin shouldn't be introduced at all.

In that case I'm not exactly sure what is the objection against having this mixin? Given that it has some benefit (which appears minor for some people, and substantial for others), and no cost (anyone who has a question why there are two mixins can read the doc-comments and find the answer).

[erickzanardo]

Having HasGameReference means that we would deprecate HasGameRef eventually, which we won't because of the reasons mentioned here, specially because of this one mentioned by Renan:

renaming the core API has some serious trade-offs for example: making existing tutorial blogs and videos totally outdated, let alone all the projects people have already deployed

Maybe this is something that we can consider rename when we start planning a Flame 2.0. But that is something for the long term future.

[spydon]

In that case I'm not exactly sure what is the objection against having this mixin? Given that it has some benefit (which appears minor for some people, and substantial for others), and no cost (anyone who has a question why there are two mixins can read the doc-comments and find the answer).

I don't understand, what are the benefits that can't be incorporated into the current mixin (except for the name changes)? We shouldn't have two ways of doing things if we can avoid it, even if we can write docs about it, users are generally bad at reading the docs well.

Maybe this is something that we can consider rename when we start planning a Flame 2.0. But that is something for the long term future.

Agree.

[st-pasha]

I'll try to summarize the state of the discussion so far, so that it is easier to move forward:

1. I believe we all agree that, by itself, and not taking other factors into account, the name change gameRef -> game is good. There is a difference in opinion of how much of a benefit this creates, but at least no one had said that the old name was better.

2. We also definitely agree that breaking changes ought to be avoided, especially if done for minor reasons like changing a slightly imperfect name.

   • We do recognize that the current PR does not create any kind of breaking change. We want it to stay that way.

The only point of disagreement is whether there will be a substantial amount of confusion, and whether the drawbacks of that confusion outweigh the benefits of having a better name. I'm not sure how to quantify this confusion, but as far as I can see, the confusion is:

• harmless, in the sense that even if someone picks one of the two mixins at random, their code would still work the same (up to a name change);
• resolvable, meaning that if someone does see two mixins with similar names and the question of why there are two classes really bothers them, they would just go and check the documentation which would state the reasons plainly;
• non-unique: there are other places in the code base where we are doing a transition from old way of doing things to the new way, this is not a "bug", this is the way code evolution works. If the users can understand this principle, it would make their life easier.

[spydon]

The only point of disagreement is whether there will be a substantial amount of confusion, and whether the drawbacks of that confusion outweigh the benefits of having a better name. I'm not sure how to quantify this confusion, but as far as I can see, the confusion is:

* **harmless**, in the sense that even if someone picks one of the two mixins at random, their code would still work the same (up to a name change);

It won't work the same if they choose the wrong one when they are following a tutorial, due to the name changes.

* **resolvable**, meaning that if someone does see two mixins with similar names and the question of why there are two classes really bothers them, they would just go and check the documentation which would state the reasons plainly;

They don't necessarily see that there are two mixins, and just pick the wrong one and it won't work for them.

* **non-unique**: there are other places in the code base where we are doing a transition from old way of doing things to the new way, this is not a "bug", this is the way code evolution works. If the users can understand this principle, it would make their life easier.

We do recognize this, but this small change just isn't worth it since it is simply a change of naming (and the class is a very commonly used one), the whole team agrees on this.

Just like Erick said, this change can be something to be considered for v2.0, but it won't be merged with the name change before that (the code change is fine however, so that could just be put into HasGameRef instead).

[alestiago]

I believe we all agree that, by itself, and not taking other factors into account, the name change gameRef -> game is good. There is a difference in opinion of how much of a benefit this creates, but at least no one had said that the old name was better.

Fully agree!

I'm in favour of the rename and fully agree with @st-pasha . The current naming is not great since it violates effective dart (https://dart.dev/guides/language/effective-dart/design#avoid-abbreviations).

I'm all in in deprecating HasGameRef for HasGameReference.

In addition, I must confess that when starting out in Flame, the first time I read gameRef I thought that it was somewhat different than game. The new naming solves this since it is clear that game is game.

[spydon]

Fully agree!

I'm in favour of the rename and fully agree with @st-pasha . The current naming is not great since it violates effective dart (https://dart.dev/guides/language/effective-dart/design#avoid-abbreviations).

I'm all in in deprecating HasGameRef for HasGameReference.

It will be deprecated, but not pre-v2. This discussion is already settled. We can create a ticket for it (with a v2 label) so that we don't forget about it.

What's left to do for this PR to get merged is to move the new code into the old mixin in a non-breaking way, could you do that @st-pasha?

[alestiago]

It is very nice that this change will get in! Great news 🔥💙

Thanks @st-pasha for working on it!

[kurtome]

It will be deprecated, but not pre-v2.

No one asked me, but this is the way. Tracking breaking changes and targeting them for v2 (possibly years out, I assume) is definitely the ideal for a project with this many users

[spydon]

No one asked me, but this is the way. Tracking breaking changes and targeting them for v2 (possible years out, I assume) is definitely the ideal for a project with this many users

Your opinion is always valued @kurtome, and that's a good point actually, we need to start tracking what we want to put in V2.

[st-pasha]

we need to start tracking what we want to put in V2.

Perhaps we could have an issue with a task list of things, and then when the time comes we could quickly convert those task items into issues with a press of a button.

[st-pasha]

@spydon I've merged HasGameReference into the HasGameRef.

[munsterlander]

@spydon @st-pasha With this being merged into main, value_route.dart in doc\flame\examples\lib is throwing errors. I have synced my fork and ran pub get just in case for that yaml file, but I am getting errors like:

HasGameReference - Classes can only mix in mixins and classes.
Missing variable type for 'score'.
Try adding an explicit type, or remove implicit-dynamic from your analysis options file.
Undefined name 'game'.
Try correcting the name to one that is defined, or defining the name.

Surely I am missing something, right? As it stands now, docs will not build on any updated branch. Reverting the file to the prior state of https://github.com/flame-engine/flame/commit/12ce270b9b3102b6a9bb1f468369a4fce1e064e6 does solve the issue though.

[spydon]

Surely I am missing something, right? As it stands now, docs will not build on any updated branch. Reverting the file to the prior state of 12ce270 does solve the issue though.

I don't see any errors regarding this, try to run melos clean and then melos bs.

[munsterlander]

Let me try that again.

[munsterlander]

Solved. Odd that I was able to get away without using melos for so long until all of a sudden, but none-the-less.