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

Migration page from "old" notifiers to "new" notifiers #2793

Closed
wants to merge 41 commits into from
Closed

Migration page from "old" notifiers to "new" notifiers #2793

wants to merge 41 commits into from

Conversation

lucavenir
Copy link
Contributor

@lucavenir lucavenir commented Aug 4, 2023

Aims to fix #2630.

About "from ChangeNotifier / StateNotifier to Notifier / AsyncNotifier":

  • Cross-link this document in the Provider migration guide, in the section talking about "Use ChangeNotifierProvider at first".
  • Mention that StateNotifier.addListener and .stream aren't present inside Notifier and help migrating.
  • StateNotifier.debugState no longer exists. Document how to work around that.
  • Document that Async/NotifierProviders do not receive a "ref" parameter.
  • Document that the Async/Notifier type is bound to autoDispose/family features.
  • Document how Notifiers are not disposed on ref.watch, ref.invalidate, etc.
  • Document how there is no-longer a "mounted" property, as a boolean wouldn't quite work
  • Document how to override "dispose" (use ref.onDispose instead)
  • Document how to run logic after "build" (i.e. after initialization)
  • [ ] Showcase a simple ChangeNotifier to Notifier migration
  • Showcase a simple ChangeNotifier to AsyncNotifier migration

About AsyncValue.when:

  • Document that e.g. you can obtain even three different values from a single AsyncValue
  • Document how to correctly use valueOrNull, hasError, hasData, asError, isLoading to intercept various states

About ProviderObserver:

  • ProviderListener is not a thing anymore: use ProviderObserver instead

@codecov
Copy link

codecov bot commented Aug 4, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.24%. Comparing base (cd0e1eb) to head (98cca35).

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##           rework-flow    #2793   +/-   ##
============================================
  Coverage        95.24%   95.24%           
============================================
  Files               53       53           
  Lines             2252     2252           
============================================
  Hits              2145     2145           
  Misses             107      107           

@lucavenir
Copy link
Contributor Author

Hey @rrousselGit this is a first draft, could you review the flow. If you like it, corrections, etc.?

Thanks ❤️

@lucavenir lucavenir marked this pull request as ready for review August 4, 2023 17:21
@rrousselGit
Copy link
Owner

@lucavenir It's help if you could fix the CI, such that the deploy preview works :)

@lucavenir
Copy link
Contributor Author

@rrousselGit I didn't realize there was a strict analysis check, even on docs' snippets. It should be OK now

@lucavenir
Copy link
Contributor Author

Ok I am done for this weekend.

On a side note, I'd love to have some inputs to the remaining paragraphs. I am unsure I can complete them at 100% by myself without some further reading, or explanation.

@rrousselGit
Copy link
Owner

I didn't realize there was a strict analysis check, even on docs' snippets. It should be OK now

It's the whole point of having them in Dart files :)
It enables error/deprecated checks or make sure that they respect riverpod_lint & stuff

@rrousselGit
Copy link
Owner

I'll look at it in depth on Monday

@lucavenir
Copy link
Contributor Author

lucavenir commented Aug 11, 2023

Hey @rrousselGit, how can I access the preview build of this PR, so that I can share it with others? I've had curious people requesting me to read this section on preview 😝

@rrousselGit
Copy link
Owner

This is great! But overall I think this is trying too much to explain the syntax and less-so to explain how to migrate.
There are already pages explaining how Notifiers work. What folks are missing is steps to migrate.

This feels less like a "How to migrate from StateNotifier to [Async]Notifier" and more like "The differences between StateNotifier and [Async]Notifier".

There are a few lines at the very end with steps to migrate, but the ratio between explanation/steps is wrong I think

@rrousselGit rrousselGit deleted the branch rrousselGit:rework-flow August 29, 2023 07:40
@lucavenir
Copy link
Contributor Author

Dang, the PR got closed and now I can't commit your suggestions. Did this get closed because it's inherently wrong?

@rrousselGit
Copy link
Owner

Oh that's because I merged rework-flow to docs-v2 I think

@rrousselGit
Copy link
Owner

Yeah I can't reopen it. Mind maybe a new PR and target the docs-v2 branch?

@lucavenir
Copy link
Contributor Author

Yeah no problem. I'll add your suggestions by hand, and then open a new PR

@lucavenir
Copy link
Contributor Author

On which branch you want me to merge, tho?

@rrousselGit
Copy link
Owner

docs-v2 is fine

@lucavenir
Copy link
Contributor Author

Oh god: I tried to merge docs-v2 into my fork, but there's hundreds of conflicts 😕 What would you do if you were me?

@rrousselGit
Copy link
Owner

Not sure what those conflicts are?

@rrousselGit
Copy link
Owner

There were no conflict between your PR and rework-flow

And there were no conflict between rework-flow and docs-v2 either, nor with master

@lucavenir
Copy link
Contributor Author

lucavenir commented Aug 29, 2023

Uh... besides the sidebar, quickstart.mdx and some meta files... mostly, generated .g.dart files, but also some .dart files used in the documentation.

I've reviewed them by hand one by one now, it should be fine I'm not sure what the best practice is

@rrousselGit
Copy link
Owner

For generated files, just run the code-generator again (melos run generate at the root to do it everywhere)

For other files, pick whatever you feel is appropriate. I'll see it in the PR anyway

@lucavenir
Copy link
Contributor Author

Okay. Thank you 😄

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.

2 participants