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

[PR] Using Riverpod and REST API in Phoenix #28

Merged
merged 32 commits into from
Feb 2, 2023
Merged

Conversation

SimonLab
Copy link
Member

@SimonLab SimonLab commented May 26, 2021

ref: #18
and secondarily #27

@SimonLab SimonLab self-assigned this May 26, 2021
@codecov
Copy link

codecov bot commented May 26, 2021

Codecov Report

Merging #28 (08286cf) into master (deca900) will not change coverage.
The diff coverage is 100.00%.

❗ Current head 08286cf differs from pull request most recent head 11d5e1d. Consider uploading reports for the commit 11d5e1d to get more accurate results

@@            Coverage Diff            @@
##            master       #28   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            6         6           
  Lines           84        84           
=========================================
  Hits            84        84           
Impacted Files Coverage Δ
lib/models/task.dart 100.00% <100.00%> (ø)
lib/screens/completed_tasks/completed_tasks.dart 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@nelsonic
Copy link
Member

nelsonic commented Nov 8, 2022

@LuchoTurtle can you please pick this up after you have looked into: #27 (comment) 🙏

@LuchoTurtle
Copy link
Member

LuchoTurtle commented Nov 28, 2022

I'm taking this up as part of dwyl/product-roadmap#40. This PR is a bit outdated so I had some trouble having to get it working with newer Flutter versions. Had to work on upgrading the dependencies, changing Podfiles, gradle and AndroidManifest.xml.

The tests don't work on my computer, though.

I'm looking at doing a semi-overhaul to this repo, to update it and using Riverpod for state management, drift to save local files and, according to dwyl/product-roadmap#40, add a Phoenix backend. I'll be using Riverpod here but for the mvp we should consider Bloc -> check my comment #27 (comment)

Though we ought to discuss this further on tomorrow's standup.

Still, there is work to be done until then so I'll be doing this.

@LuchoTurtle
Copy link
Member

LuchoTurtle commented Nov 29, 2022

@nelsonic

I fixed some bugs where the user couldn't create todos and properly check them. The tests weren't working and I got one fixed, which took too much time for what it was worth.

It feels like I'm fighting an uphill battle with this because I'm dealing with outdated specs and generated files on both Android and Apple, so there is a lot of unexpected behavior with this. This PR deals with an outdated version of Riverpod and the widgets and providers are built using ChangeNotifierProvider, which is not recommended by the library creators. Getting this to run was quite painful as it was still using Android Embedded v1 and older Flutter components. The Riverpod version was outdated as well, it's already on 2.x.x and these changes had a 0.x.x version.

Getting a single integration test to run was time-consuming, having resulted in lots of workarounds just to get it working. For example, here's a snippet testing a simple widget so it was workable when calling tester.pumpWidget().

    Widget testWidget = Localizations(
        locale: const Locale('en', 'US'),
        delegates: const <LocalizationsDelegate<dynamic>>[
          DefaultWidgetsLocalizations.delegate,
          DefaultMaterialLocalizations.delegate,
        ],
        child: MediaQuery(
            data: new MediaQueryData(),
            child: Material(
                child: ProviderScope(
                    overrides: [
                  todolistProvider.overrideWith((ref) {
                    return todoList;
                  })
                ],
                    // Our application, which will read from todoListProvider to display the todo-list.
                    // You may extract this into a MyApp widget
                    child: Scaffold(body: TodoListWidget())))));

I feel like this is not sustainable if I want to get this out quickly. I'm bringing this up in tomorrow's standup but I feel like I should revamp this repo with a newly-generated project and follow Riverpods updated recommendations. I don't want any code to go to waste, so I'll borrow some styling and code from this PR. But I don't think continuing this PR with the current outdated project is the way to go.

Additionally, I think this repo gains importance as it is related to the mvp.

It's up to you though, this is my recommendation as I feel like I'm wasting more time than what it's worth. If you give me the green light, I'll continue to work on this PR but "start from scratch" (not really but you get my point).

@nelsonic
Copy link
Member

Thank you for this very insightful comment on Flutter project maintainability. 🙏
Please go for it if you need to nuke the project and start from scratch. 💥

@LuchoTurtle
Copy link
Member

Going to go ahead and start from scratch but using the same pieces of code written by @SimonLab on the new one so his efforts don't go to waste 👍

@nelsonic
Copy link
Member

@LuchoTurtle Thanks for sharing your progress + plans. Very insightful. 💡 Good luck on your quest. 👌

@nelsonic
Copy link
Member

nelsonic commented Dec 3, 2022

@LuchoTurtle nice one. thanks for the update. hope you have a great weekend. ☀️ 🙌

@LuchoTurtle
Copy link
Member

I will now resume this task, following the creation of dwyl/phoenix-todo-list-tutorial#60.
Even though it's not merged yet, I'll run it on localhost to try and get it properly working.

@LuchoTurtle LuchoTurtle changed the title Use Riverpod as state management tool [PR] Using Riverpod and REST API in Phoenix Dec 13, 2022
@LuchoTurtle
Copy link
Member

I'm basically finished with this when it comes to implementation. I have tests on my local change but I haven't committed them because they are falling and I'm pulling my hair as to why. I shall continue tomorrow.

I'm also finishing the README to make use of the API.

Created a "repositoryProvider" that is used to fetch the repository instance. We can mock "repositoryProvider" when testing.
@LuchoTurtle
Copy link
Member

Had to fix the tests that were failing. Took a while to figure out how I could mock HTTP requests when using providers from Riverpod. I had to change a bit how I was using providers.
For this, I had to create a repositoryProvider which exposes the repository (the service used to make HTTP calls) and use it on other providers. This was the only way I could find to minimize code change and be able to mock HTTP requests when testing.

All these changes were reflected in the README as well, which is basically finalized at this point.

Now I'm going to be focusing on finishing the tests so I get back to 100% coverage and push the changes.

@LuchoTurtle
Copy link
Member

This should be ready for review. Added the tests, fixed typos and re-wrote the README according to changes needed.

Copy link
Member

@nelsonic nelsonic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent Work @SimonLab & @LuchoTurtle 🎉

@nelsonic nelsonic merged commit 575a8ba into master Feb 2, 2023
@nelsonic nelsonic deleted the riverpod-#27 branch February 2, 2023 16:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting-review enhancement New feature or request
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants