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

Change RiverpodComponentMixin.onLoad to return FutureOr<void> #2961

Closed
tibotix opened this issue Jan 5, 2024 · 3 comments · Fixed by #2964
Closed

Change RiverpodComponentMixin.onLoad to return FutureOr<void> #2961

tibotix opened this issue Jan 5, 2024 · 3 comments · Fixed by #2964

Comments

@tibotix
Copy link
Contributor

tibotix commented Jan 5, 2024

What could be improved

The function signature of RiverpodComponentMixin.onLoad could be changed from void onLoad() to FutureOr<void> onLoad.

Why should this be improved

For example, when trying to mixin RiverpodComponentMixin to a TextBoxComponent, a function signature mismatch occurs, as RiverpodComponentMixin.onLoad has signature void onLoad and TextBoxComponent.onLoad has signature Future<void> onLoad.
I know i could just make a separate riverpod aware component that extends Component (which in fact has the signature FutureOr<void> onLoad) (such as in the documentation), but i would like to have the provider logic inside the actual component.
Here is some code to see what i am talking about.

class MyTextBox extends TextBoxComponent with RiverpodComponentMixin { // ERROR: can't mixin  (signature mismatch)
  @override
  void onMount() {
    addToGameWidgetBuild(() {
      ref.listen(myProvider, (previous, next) {
        // do some things such as updating text contents
      });
    });
    super.onMount();
  }
}

Any risks?

I don't know if having the RiverpodComponentMixin attached to an actual component other than the plain Component class poses any problems, but i don't think so.
Theoretically this is an API change, since the signature changes. However, this signature change will not break any code, because the way FutureOr<T> is defined.

More information

i could do a PR if this is considered useful. But for such a small change i wouldn't be bothered if you would change it yourself real quick.

@spydon
Copy link
Member

spydon commented Jan 6, 2024

@markvideon

@tibotix tibotix changed the title Change RiverpodComponentMixin.onLoad to return FuturOr<void> Change RiverpodComponentMixin.onLoad to return FutureOr<void> Jan 6, 2024
@markvideon
Copy link
Contributor

@markvideon

Seems like a totally sensible improvement to me!

spydon added a commit that referenced this issue Jan 7, 2024
…void> (#2964)

This PR changes the return type of RiverpodComponentMixin.onLoad to
match that of Component.onLoad.

This resolves #2961 which describes an error preventing use of
RiverpodComponentMixin on Component subclasses that return a Future in
their implementation of onLoad due to a signature mismatch.


Closes #2961.

---------

Co-authored-by: Lukas Klingsbo <lukas.klingsbo@gmail.com>
@spydon
Copy link
Member

spydon commented Jan 7, 2024

Released in flame_riverpod v5.1.5

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants