fasky-software / flutter_widgetkit

Flutter library for iOS Widgets Extensions. Integrate a Widget into your App πŸπŸ“±
MIT License
260 stars 23 forks source link

Migrate package to null-safety #10

Closed ThrowJojo closed 3 years ago

ThrowJojo commented 3 years ago

First of all thanks so much for creating this package. Working with iOS 14 widgets is so much faster without having to write all the boilerplate for reloading timelines or setting NSDefault values on the native side.

I ran the migration tool for null-safety and this is what it came up with(the changes are really minimal because it doesn't depend on anything in pubspec.yaml).

One thing I'm not really sure about is restricting the flutter version to >=2.0.0 under environment in pubspec.yaml. I checked a few other packages and some seem to make the restriction while others don't.

Ahmadre commented 3 years ago

/cc @tomLadder

dancamdev commented 3 years ago

It would be awesome to have this PR reviewed and merged!

ThrowJojo commented 3 years ago

I just noticed the migration tool changed removeItem to returnbool? instead of a bool. It'd probably be better if it wasn't null-able. I'll fix this if the Tom agrees it's better. πŸ‘

I checked a couple of the packages in https://github.com/flutter/plugins/tree/master/packages and they all seem to have their Flutter environment constraints set to flutter: ">=1.12.13+hotfix.5" so maybe there's no need to raise that constraint after all.

tomLadder commented 3 years ago

Hi @ThrowJojo, thanks for the contribution! I also think it would be better if it's not null-able. To be honest I'm not really sure about the version increase. Maybe someone has more experience than me in this regard.

dancamdev commented 3 years ago

I agree there shouldn't be a need to raise the constraint, and looking at the code it is safe to make removeItem return a bool, @ThrowJojo are you working on the fix? Otherwise, I'm available too

ThrowJojo commented 3 years ago

@dancamdev Yeah I'm on it, I think it's done I'm just going to try using my fork with my project to check everything is in order again. πŸ‘

ThrowJojo commented 3 years ago

Okay everything seems to be working πŸ‘

I think leaving the constraints as they are is the right course. I tried a couple of different combinations and I think if the constraints are left how they are people who have upgraded to Flutter 2.x.x but haven't turned on null-safety yet will still be able to use the latest (null-safety) version of the package.

If any body has a spare couple of minutes, could somebody try running the example app on a simulator and making sure it works okay with the widget? I got an m1 this week and I can run the example app on the simulator but the widget isn't showing up for some reason πŸ˜‚. My project has the same issue but I could at least test that on a physical device(code-signing etc.). It should work fine because I had no issues with the project depending on my fork.

/cc @tomLadder

dancamdev commented 3 years ago

@ThrowJojo I'm testing on a non-M1 Macbook, I'll let you know in a minute

dancamdev commented 3 years ago

It's working perfectly!

cc. @tomLadder

Screenshot 2021-03-17 at 19 00 04
ThrowJojo commented 3 years ago

Thanks for checking! Was almost gonna bust out my 15-inch Macbook Proβ„’.

dancamdev commented 3 years ago

Sorry it took me a bit, I was really busy in the last few days, it would be great to see this one merged now πŸ™Œ

Ahmadre commented 3 years ago

Sorry it took me a bit, I was really busy in the last few days, it would be great to see this one merged now πŸ™Œ

Now it looks great πŸ‘πŸΌ this is ready for release null-safety and not only pre-release null-safety.

LGTM

tomLadder commented 3 years ago

Just published v1.0.3 πŸš€ Thank you all for the contribution!

dancamdev commented 3 years ago

Awesome, Thank you @tomLadder, and thank you @ThrowJojo for the hard work!

ThrowJojo commented 3 years ago

Thanks for merging this and pushing a new release @tomLadder! Thank you too @dancamdev and @Ahmadre for checking everything. πŸš€