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

feat: Web support #730

Closed
wants to merge 11 commits into from
Closed

Conversation

mirland
Copy link

@mirland mirland commented Jan 27, 2023

Right now sqflite_common_ffi_web is in an experimental state, but if we add the factory, we can support the web optionally.

What do you think?

Currently, floor doesn't support web and we cannot update the factory creator, so if we want to support web we have to change the generated code (or duplicate code), and it's not a good idea.
This pr adds the web factory

@dkaera
Copy link
Collaborator

dkaera commented Jan 27, 2023

wow, looks good, did you test it out?

@mirland
Copy link
Author

mirland commented Jan 27, 2023

Yes doing these changes manually I could make my application work. I tested in this branch, but I did floor changes just locally

@dkaera
Copy link
Collaborator

dkaera commented Jan 31, 2023

Yes doing these changes manually I could make my application work. I tested in this branch, but I did floor changes just locally

awesome, I'll prepare the project update PR, because the current one contains a lot of changes in the .lock file, which doesn't related to the main change.

dkaera
dkaera previously approved these changes Feb 11, 2023
@dkaera
Copy link
Collaborator

dkaera commented Feb 11, 2023

Hey @mirland
ubuntu job has broken
I fixed it in my PR
let's merge your changes after mine

ThomasMiddel
ThomasMiddel previously approved these changes Apr 18, 2023
@mirland mirland dismissed stale reviews from ThomasMiddel and dkaera via 405aad7 April 18, 2023 13:24
@codecov
Copy link

codecov bot commented Apr 21, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 0.00%. Comparing base (5658ea0) to head (aa095b6).

❗ Current head aa095b6 differs from pull request most recent head 9c19ecb. Consider uploading reports for the commit 9c19ecb to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           develop   #730       +/-   ##
==========================================
- Coverage    91.92%      0   -91.93%     
==========================================
  Files           11      0       -11     
  Lines          223      0      -223     
==========================================
- Hits           205      0      -205     
+ Misses          18      0       -18     
Flag Coverage Δ
floor ?

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mirland
Copy link
Author

mirland commented May 8, 2023

Hi @dkaera, do you have any feedback for my PR? I don't know if I can do something to merge it

@dkaera
Copy link
Collaborator

dkaera commented May 9, 2023

Hi @dkaera, do you have any feedback for my PR? I don't know if I can do something to merge it

the coverage check blocks this PR, so you need to add the corresponding test for the newly added condition first

@mirland
Copy link
Author

mirland commented May 9, 2023

the coverage check blocks this PR, so you need to add the corresponding test for the newly added condition first

Hi @dkaera , can you suggest to me how can I do that, in this case, there's no test to do, I mean, if we want to do that we should like mock the platform and check the factory. I checked your tests and that method seems to be not tested and there's not something similar

@mirland
Copy link
Author

mirland commented Jun 5, 2023

@dkaera can you suggest to me what kind of test should I include?

@dkaera
Copy link
Collaborator

dkaera commented Jun 5, 2023

@dkaera can you suggest to me what kind of test should I include?

ah, I'm sorry, I can't help you now, because of the heavy workload.

@mirland
Copy link
Author

mirland commented Jun 5, 2023

Ok, np, no rush. Then when you have time, please check it because I changed only 3 lines and I think I cannot do a test for that.

image

Thanks!

@dkaera
Copy link
Collaborator

dkaera commented Jun 5, 2023

Ok, np, no rush. Then when you have time, please check it because I changed only 3 lines and I think I cannot do a test for that.

image

Thanks!

I think we can mock Platform value and verify what the instance was factory created.

@stefanosiano
Copy link

any news on this?
No rush, i'd just like to try floor instead of sqflite in my project, which runs on desktop and web

@mirland
Copy link
Author

mirland commented Sep 27, 2023

Hey, not really, I couldn't do the test. I mean, If I wanted to add it I had to redefine some things, and I didn't find the value of that only for this test. However, if you want to try the database on the web it's not had. You have to add the dependency, and then redefine the factory in your app database.

Here you have an example: https://github.com/xmartlabs/fluttips/blob/main/lib/core/source/database.dart#L24

After that you have to set up the binaries, and that's all

@stephanmantel
Copy link
Contributor

stephanmantel commented Mar 8, 2024

Ok, np, no rush. Then when you have time, please check it because I changed only 3 lines and I think I cannot do a test for that.
image
Thanks!

I think we can mock Platform value and verify what the instance was factory created.

Is it possible to mock Platform and kIsWeb? I found no support for static mocking using Mockito.

We could also resort to a wrapper object, but I feel like that misses the point a bit, as the real logic you'd want to test is mocked away. What do you guys think, is it necessary to test this?

# Conflicts:
#	example/android/gradle/wrapper/gradle-wrapper.properties
#	example/pubspec.lock
#	floor/pubspec.lock
#	floor/pubspec.yaml
@dkaera dkaera mentioned this pull request Mar 16, 2024
@dkaera
Copy link
Collaborator

dkaera commented Mar 16, 2024

Hey guys
PR #815 should resolve the analyzer issues and unlock the merge of the current PR
Lets push it to development brunch then we can merge @mirland changes to support web
I tested it locally - everything works fine 💪💪💪

@stephanmantel
Copy link
Contributor

Hey guys PR #815 should resolve the analyzer issues and unlock the merge of the current PR Lets push it to development brunch then we can merge @mirland changes to support web I tested it locally - everything works fine 💪💪💪

I love it, let's get it merged and great idea to have brunch ;)

@hendrikvdkaaden
Copy link
Contributor

Is this PR still relevant, since it's merged to #819?

@dkaera
Copy link
Collaborator

dkaera commented Apr 30, 2024

@hendrikvdkaaden yes we can close this PR

@dkaera dkaera closed this Apr 30, 2024
@dkaera
Copy link
Collaborator

dkaera commented Apr 30, 2024

the changes were picked up and integrated separately

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

6 participants