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

Flutter 2 support #345

Open
zmeggyesi opened this issue Mar 30, 2021 · 13 comments · May be fixed by #346
Open

Flutter 2 support #345

zmeggyesi opened this issue Mar 30, 2021 · 13 comments · May be fixed by #346

Comments

@zmeggyesi
Copy link

With Flutter 2 being de facto mandatory by Google upgrading its most important packages, w_transport should be upgraded too. However, I see that some of the Workiva-internal packages are holding back the upgrade, most obviously w_common.

Can we get a Flutter 2-compatible version release, even if without the Null Safety features for now?

@travissanderson-wf
Copy link

Hey @zmeggyesi, we are definitely interested in updating this. I replied in your w_common issue but in addition to w_common, one snag for w_transport is proper support of Flutter Web might be a bit tricky. If you're not using Flutter Web, it will be much easier!

@zmeggyesi
Copy link
Author

Hey, @travissanderson-wf,

Yes, I saw that reply, and it lifted my spirits by itself. I'm not planning on Flutter Web just yet (I have a few more dependencies to sort out before I can get to that, but it's definitely on the roadmap in the mid-term), if that helps. For now, I just want to make the leap to Flutter 2, NNBD/nullsafety be damned, to be able to keep up with some mandatory upgrades to the Firebase packages that will be coming due in April.

@travissanderson-wf
Copy link

travissanderson-wf commented Apr 1, 2021

Hi, I just verified I can now use w_transport (since the new release of w_common) in Flutter (edit: Flutter 2) successfully. Null safety support may be a ways out since we're dependent on some upstream improvements (isn't everyone!?). Let me know if you have any issues.

@zmeggyesi
Copy link
Author

Let me give it a shot come Tuesday (holidays here until then) and see if I manage to get past this block.

Thanks for getting on the matter so quickly!

@zmeggyesi
Copy link
Author

Almost there, @travissanderson-wf ...

It looks like a great many packages have upgraded their http and uuid dependencies (the latter through the logging package), which means they are now incompatible with w_transport,a nd being core packages, they are probably quite widespread in the ecosystem.

I tried upgrading the version numbers for these, but now over_react ... well, overreacts with an analyzer dependency:

Because analyzer >=0.32.4 <0.39.5 depends on crypto >=1.1.1 <3.0.0 and analyzer >=0.39.5 <0.41.2 depends on crypto ^2.0.0, analyzer >=0.32.4 <0.41.2 requires crypto >=1.1.1 <3.0.0.
And because uuid >=3.0.0 depends on crypto ^3.0.0 and over_react >=3.5.2 depends on analyzer >=0.35.0 <0.40.0, uuid >=3.0.0 is incompatible with over_react >=3.5.2.
So, because w_transport depends on both over_react >=3.12.0 <5.0.0 and uuid ^3.0.3, version solving failed.

Though it's a trivial change, do you want me commit my changes?

@travissanderson-wf
Copy link

Sure, a PR would facilitate the conversation better. Just looking at the pubspec.yaml, the uuid dep is only a dev dependency so that shouldn't impact your ability to consume w_transport. I don't see a dep on http but probably there is a transitive one.

@zmeggyesi
Copy link
Author

@travissanderson-wf PR submitted, no matter how trivial - linked it via keyword so it's easier to find.

@evanweible-wf
Copy link
Contributor

Thanks for looking into this and getting it started, @zmeggyesi! I took a quick look at it, and since over_react is just used for examples, we could probably comment it out and make the example code its own package temporarily. After that, we'd just need to get updated versions of:

I'll work on getting code review from my team on the two open PRs so that we can move this along. Thanks for your patience!

@zmeggyesi
Copy link
Author

Thanks for moving it along, @evanweible-wf - the fact that it's moving along is a good sign already.

Let me know if you need me to do anything with this PR or just feel free to port the change and close since it's such a trivial matter.

@zmeggyesi
Copy link
Author

@evanweible-wf I made some headway in unravelling the web of dependencies underlying w_transport. Notably, I got package resolution working all the way, through sockjs_client_wrapper and w_common, but be advised, I had to use heavy-handed measures to get there, especially related to dart_dev.

I'm fully aware that these changes are likely not merge-compatible as they are, but hopefully, they can serve as a jumpstart for the rest of the work.

@zmeggyesi
Copy link
Author

@evanweible-wf Has there been any movement on this front? This is really becoming a blocker for me, as it prevents even resolving my app's dependencies to see what I need to modify for Flutter 2... :(

@evanweible-wf
Copy link
Contributor

Hi @zmeggyesi. I'm sorry it hasn't been moving very fast, I'm doing my best to find time to get these packages migrated, but it's mostly happening on personal time. We've got a lot on our plate internally, but I will keep pushing to get these done.

@zmeggyesi
Copy link
Author

That's a Black Swan right there, I did not know this is on personal time as opposed to official Workiva time.

I'll try to come up with a workaround so that my app can move forward while this takes place.

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 a pull request may close this issue.

3 participants