Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Support null-safety #195

Closed
al8n opened this issue Jan 2, 2021 · 60 comments
Closed

Support null-safety #195

al8n opened this issue Jan 2, 2021 · 60 comments
Labels
Waiting for PR already being worked on. We're waiting on a PR

Comments

@al8n
Copy link

al8n commented Jan 2, 2021

Thanks for the amazing packages. Please support the null-safety version.

@FilledStacks
Copy link
Contributor

You're very welcome. I will start the null-safety migration soon.

@Sameerkash
Copy link
Contributor

@FilledStacks can I take this up?

@FilledStacks
Copy link
Contributor

@Sameerkash Yes Yes! I would not say no to any contribution like that.

I haven't looked at null-safety so I'd have to go find out what needs to be done and then do that.

Do you have an approach you'll take i.e. all parameters will be nullable unless otherwise specified. Just so I can know what to look for when reviewing the PR.

@Sameerkash
Copy link
Contributor

Sameerkash commented Jan 11, 2021

@FilledStacks think you mean, all parameters will be non-nullable unless specified ? :D

@FilledStacks
Copy link
Contributor

haha @Sameerkash yes! I'll create a new branch under feature/null-safety please make the PR to that branch when you're ready.

@FilledStacks
Copy link
Contributor

Pushed

@Sameerkash
Copy link
Contributor

Sure!! I'll keep you posted.
Will do it asap.

@Sameerkash
Copy link
Contributor

Hey @FilledStacks , just an update, I have been waiting for the migration of the dependencies, I am following the issues on them and have raised the issue where it isn't present. I'll give another update as soon the dependencies are resolved for at least one package

@FilledStacks
Copy link
Contributor

@Sameerkash awesome, thanks for the update!

@FilledStacks FilledStacks added the Waiting for PR already being worked on. We're waiting on a PR label Jan 24, 2021
@Sameerkash
Copy link
Contributor

Sameerkash commented Jan 26, 2021

@FilledStacks, there is still no package with null safe dependencies. I'm gonna try and reach out/ triage them soon.

Although one thing that's a bit out of hand is flutter_statusbarcolor in stacked_themes, it has been archived by the author here.

will you remove the dependency or should we migrate a fork and publish it?

@FilledStacks
Copy link
Contributor

@Sameerkash that's crazy. I wonder why it was archived. We should probably migrate and publish that. Or maybe even remove it for something better.

@Sameerkash
Copy link
Contributor

@FilledStacks what do you suggest? I can fork migrate and publish it, we might also have to do the same for observble_ish
because I have not gotten any reply from the maintainer.

@FilledStacks
Copy link
Contributor

@Sameerkash observable_ish you can actually remove along with the functionality.

And on the other package. I think we can for and publish that. That would be the best thing at the moment. Check if there's a status bar color that provides the same functionality maybe, then we could even use that one. Actually we can even for and point directly to the gitrepo if that's an option.

@Sameerkash
Copy link
Contributor

Sameerkash commented Jan 30, 2021

@FilledStacks I'll take a look at obserable_ish and how it can be dealt with, meanwhile, I'll fork status_bar_color, migrate it and point It to the git repo. I'll try checking out if there's any alternative available before doing that, but yeah, that would be the simplest solution.

@FilledStacks
Copy link
Contributor

Thanks a lot @Sameerkash for all the efforts. I really appreciate it. The goal is to completely remove observable_ish from the package at some point. I'd have to write the migration guide for that as well but that shouldn't take too long.

@Sameerkash
Copy link
Contributor

Sameerkash commented Jan 31, 2021

@FilledStacks no problem, it's a great learning opportunity for me and I love working on the framework I love the most.

I'm not yet aware as to what obserbale_ish is actually doing, so will take me some time to find and remove it without affecting the code. But I'll definitely give it a try and get back to you with any doubts. Meanwhile, if you have any external references that can help me, that'd be great.
And what would you suggest we use instead of observable_ish ?

So before that, I'll do the flutter_statusbar migration as that'll be an easier task to finish.

@FilledStacks
Copy link
Contributor

FilledStacks commented Feb 2, 2021

@Sameerkash yes tackle status bar first.

you can hold off on observable_ish for now. I'd like to provide the user with sufficient time to migrate their code by just deprecating it. We might have to fork and manage it ourselves going forward.

I'll have to make some time to look at that.

@Sameerkash
Copy link
Contributor

Sameerkash commented Feb 2, 2021

@FilledStacks I saw the obersvable_ish is only being used to compare types.

How can I recreate that logic while removing the type ?
Any pointers ?

@Sameerkash
Copy link
Contributor

Sameerkash commented Feb 2, 2021

@FilledStacks, I've migrated flutter_statusbarcolor, although linter complains publishable packages can't have git dependencies. So will have to publish the package, but I'm not a publisher yet. So, will try to get a domain off some student offers. 😅

is there no way to publish on pub without a domain?

Another blocker is shared_preferences for stacked_themes.
I'm working on that and waiting for its dependencies to migrate, which should be done soon.

@FilledStacks
Copy link
Contributor

@Sameerkash you don't need a domain. You can just publish your package with your gmail account. I've done that before. I only recently moved it over to Filledstacks.com

@Sameerkash
Copy link
Contributor

Sameerkash commented Feb 3, 2021

@Sameerkash you don't need a domain. You can just publish your package with your gmail account. I've done that before. I only recently moved it over to Filledstacks.com

What do I give in the publisher domain ? I tried my e-mail it didn't work.

Screen Shot 2021-02-03 at 8 17 01 PM

Can't get past this, says enter a valid domain

@FilledStacks
Copy link
Contributor

You can give any domain I think. I've never seen that.

Try Sameera.com and see if it works. I know I didn't have to verify anything.

@Sameerkash
Copy link
Contributor

@FilledStacks , nope, Any domain doesn't work, it opens up google search console and asks to verify. And confirmed the same on twitter.

Anyway, I have a few free domain plans from the github student pack. I'll use it and publish.

@FilledStacks
Copy link
Contributor

@Sameerkash I can publish it too if that'll be easier for you?

@Sameerkash
Copy link
Contributor

@Sameerkash I can publish it too if that'll be easier for you?

@FilledStacks no issues, I think this will be an opportunity to finally create a proper domain and also get started with publishing packages.

Because I want to soon enough.

@Sameerkash
Copy link
Contributor

@FilledStacks I've published the fork of flutter_statsbarcolor and I've been working on migrating shared_preferences too and The PRs are up on the flutter/plugin repo should get merged soon.

So stacked_themes should be migrated soon since these are the two dependencies.

Apaprt from this. if you give me a lead on stacked with observable_ish, how to remove the dependency. We can work on that soon too.

@FilledStacks
Copy link
Contributor

Awesome @Sameerkash it's quite a lot of work. I didn't think it would take this long. I appreciate all the effort and the work you've put in.

In terms of observable_ish I don't know what the move is there. I'd probably move that into the stacked repo as apart of it. Like apart of the stacked src folder and put it in src/reactive. Maybe remove some of the code that's in there.

But I don't know if that's legal. We could also simply use the implementation that they have as it is and then rename it on our side.

@Sameerkash
Copy link
Contributor

@FilledStacks I just got a lead on observable_ish the author told he'd accept a PR if I make it. So I'll migrate it soon and maybe we can use that ?

@FilledStacks
Copy link
Contributor

@Sameerkash yes, we can use that for now. That would be the least effort.

@IchordeDionysos
Copy link

The new Dart version with null-safety was just released 😍
https://medium.com/dartlang/announcing-dart-2-12-499a6e689c87

@alvinvin00
Copy link
Contributor

@Sameerkash thanks! That's awesome. Super excited to get that ready.

Didn't think it was going to be such a long process. there's been a lot of stacked updates over the past few weeks so hopefully I can sort out that null-safety when we start the merging process.

Now that you mentioned it, stacked_hooks to Null Safety next :)

@CoryADavis
Copy link
Contributor

The new flutter migrate tool should help a lot.

@Sameerkash
Copy link
Contributor

The new flutter migrate tool should help a lot.

Yes I've been on it for a month now, the only blockers are the dependencies, some dependencies are not just migrating,
I stepped in myself and made PRs for those too like shared_prefs, statusbarcolor and obserbale_ish.

That's the only reason its taking so long, I'm trying my best to migrate it soon. Hopefully, it should be done soon.

@Sameerkash
Copy link
Contributor

@Sameerkash thanks! That's awesome. Super excited to get that ready.
Didn't think it was going to be such a long process. there's been a lot of stacked updates over the past few weeks so hopefully I can sort out that null-safety when we start the merging process.

Now that you mentioned it, stacked_hooks to Null Safety next :)

Stakced_hooks is already migrated since it just extends BaseWidget with HookWidget. You can see it in the PR I've raised

@Sameerkash
Copy link
Contributor

@FilledStacks , I was trying to migrate stacked_firebase_auth and the facebook login dependency hasn't been migrated.
When I raised an issue, someone replied saying the package is obsolete and probably won't be updated.

Any reason you picked the current one over flutter_login_facebook This seems to have better support and is also migrated.

@Sameerkash
Copy link
Contributor

Sameerkash commented Mar 6, 2021

some updates,
Edit :
Stacked is alsmot migrated, just one widget test not passing. Finding fixes.

stacked themes are almost done, some tests are failing which I'm trying to resolve because Mockito does not work well with null -safety and they have to generate some mock overrides.

@merfire
Copy link
Contributor

merfire commented Mar 9, 2021

@FilledStacks @Sameerkash
I am not sure if this is the right place to discuss this, but since it was mentioned, I think stacked_firebase_auth should use flutter_facebook_auth, which is recommended in official docs for firebase_auth facebook social auth. It is much better supported than the currently used flutter_facebook_login package which was last updated Sep 22, 2019.

@FilledStacks
Copy link
Contributor

@merfire I'll look into that before making the next video since we're doing social auth in the next episode of the boxtout series. Thanks for bringing it up.

@Miamoto-Musashi
Copy link

does this mean also the web will be supported?

@prochazkaa
Copy link

Is there anything new about migration on null-safety?

@Sameerkash
Copy link
Contributor

Is there anything new about migration on null-safety?

@prochazkaa this is the main blocker right now, Jaguar-dart/observable_ish#4
And there's one more test that's failing which I'm unable to figure out at the moment, if someone could help me, stacked would be migrated soon.

@merfire
Copy link
Contributor

merfire commented Mar 13, 2021

Maybe it is a good time to deprecate observable_ish, and fully removed it in null-safe version of stacked?
As I understood here #265, recommended way of using ReactiveServiceMixin is with notifyListeners, observable_ish isn't really needed anymore.

@prochazkaa
Copy link

What's the plan?
I would like to know if I should wait or switch to another form of status management.

@Sameerkash
Copy link
Contributor

Maybe it is a good time to deprecate observable_ish, and fully removed it in null-safe version of stacked?
As I understood here #265, recommended way of using ReactiveServiceMixin is with notifyListeners, observable_ish isn't really needed anymore.

If, we're gonna do this right now, I should maybe wait and emigrate and then push

@FilledStacks
Copy link
Contributor

@Sameerkash I'm happy with removing observable_ish from stacked.

@Sameerkash
Copy link
Contributor

Sameerkash commented Mar 17, 2021

@Sameerkash I'm happy with removing observable_ish from stacked.

@FilledStacks
I'm sorry this is taking so long, I didn't expect it to either. But things keep getting complicated. There were additional changes in get_it that I think are causing some tests to fail.

Would you want me to take this up? I'm happy to, but I might take a little bit of time as I lack understanding of how exactly obserbale_ish is integrated with stacked to make this huge change, but If you can give me some pointers, I'll dive in and try to make it happen. This might actually speed up the migration of stacked.

@FilledStacks
Copy link
Contributor

No need to apologise, you're doing this in your own time. The change for observable_is it's actually not that big. It'll just break a few things for people that are using it but it should be fine. It's only used in the ReactiveServiceMixin so just that file will be affected (if I remember correctly).

  1. Remove observable_ish from the ReactiveServiceMixin.
  2. Remove the package dependency
  3. Update the readme to remove all the mentions of using the RxValue
  4. Replace the mentions of RxValue with using notifyListeners after an update to get the viewmodel to rebuild.
  5. Add or update readme migration section at the top to mention the breaking change.

@YazeedAlKhalaf
Copy link
Contributor

Hide this, I just need to be notified on updates here 🙈🔥

@merfire
Copy link
Contributor

merfire commented Mar 22, 2021

@YazeedAlKhalaf You can subscribe to issue without leaving a comment, on the right side of the issue page there is a Notifications section with subscribe button.

@FilledStacks
Copy link
Contributor

@Sameerkash I'd like to see if I can move this along too. Please leave me an update, i'd like to continue from your work. If I can find a way to merge it into a separate branch I'm going to do that and see if I can finish up the observable_ish change.

@Sameerkash
Copy link
Contributor

@FilledStacks

My current progress is

Migrated stacked with observable_ish
And migrated Stacked_themes

The only problem is 1-2 tests are failing in each.
Apart from that seems good. If that's okay, I'll push it to the branch we already have on null - safety and we can merge it into that.

Please let me know if there's any other way I can help since I'm not being able to spend the time for this as I'm swamped with a lot of things.

@FilledStacks
Copy link
Contributor

@Sameerkash that's fine, thank you so much. Push what you have and I'll take it from here. I've merged in the latest PR code. If you've made any additional changes please make that PR to the same feature/null-safety branch and push that up.

@a43501
Copy link

a43501 commented Mar 25, 2021

Error: type '(HomeViewModel) => dynamic' is not a subtype of type '((HomeViewModel?) => dynamic)?' after update to stacked: ^2.0.0-nullsafety.2. Please help. Thank you!

#0 _ViewModelBuilderState._createViewModel
package:stacked/…/state_management/view_model_builder.dart:115

#1 _ViewModelBuilderState.initState
package:stacked/…/state_management/view_model_builder.dart:93

@Reprevise
Copy link

When will a prerelease be available for stacked_generator?

@FilledStacks
Copy link
Contributor

@Reprevise Everything is available now in a null-safe version stacked as well

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Waiting for PR already being worked on. We're waiting on a PR
Projects
None yet
Development

No branches or pull requests