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

Fix react-native example based expo #3903

Merged
merged 31 commits into from Sep 13, 2022

Conversation

msotnikov
Copy link
Contributor

@msotnikov msotnikov commented Jul 22, 2022

This PR contains

Updated example for react-native based on Expo.

Main changes at 01.09.2022:

  1. Updated versions expo@46.0.9, pouchdb@7.3.0, react@18.2.0, react-native@0.69.2
  2. Fixed sync with couchdb instanance (you need your own)
  3. Some refactoring

@pubkey
Copy link
Owner

pubkey commented Jul 22, 2022

A main reason for having these example projects, is to know when a change on the rxdb code breaks compatibility.
So I do not want to install rxdb from npm but it should use the build rxdb bundle. Also having a fixed version there would mean that someone has to come and update it on each rxdb release.

@msotnikov
Copy link
Contributor Author

Ok, changed back. Added comment in readme.md.

@pubkey
Copy link
Owner

pubkey commented Jul 22, 2022

Can you update this line https://github.com/pubkey/rxdb/blob/master/.github/workflows/main.yml#L450
So that it does use the node version from the node-version-file: '.nvmrc' like in the other ci tasks.

@msotnikov msotnikov force-pushed the fix-example-react-native-expo branch from de3ea2a to 3b419ef Compare July 23, 2022 04:20
@msotnikov
Copy link
Contributor Author

expo-cli doesn't support node 18. expo/expo-cli#4478

@pubkey
Copy link
Owner

pubkey commented Jul 23, 2022

I see.
I am not sure if we should merge this then. Because we increase the expo version but decrease the react-native version, which does not lead to any benefit.

@msotnikov
Copy link
Contributor Author

msotnikov commented Jul 23, 2022

I see. I am not sure if we should merge this then. Because we increase the expo version but decrease the react-native version, which does not lead to any benefit.

But current code don't work any case :-)

Now updated only expo-cli. I leave old expo package and it have recommendations https://docs.expo.dev/versions/latest/#each-expo-sdk-version-depends-on-a. It was downgraded to prevent notification.

I'll try to update all deps

@msotnikov
Copy link
Contributor Author

With jest-expo >= 44 tests passes, but finishes with seg fauilt (yarn and npm same). Now its works.

@pubkey
Copy link
Owner

pubkey commented Jul 25, 2022

Replication does not work because you switched from the pouchdb RxStorage to the memory RxStorage.
But the replication code uses the CouchDB replication protocol which only works with PouchDB RxStorage.

@pubkey
Copy link
Owner

pubkey commented Jul 26, 2022

Is there a reason why you switched to the memory adapter?
Because PouchDB is the only persistent RxStorage for react native, so the whole reason for the react-native example is to explain how to store data in it with the PouchDB RxStorage.

@msotnikov
Copy link
Contributor Author

msotnikov commented Jul 26, 2022

Is there a reason why you switched to the memory adapter? Because PouchDB is the only persistent RxStorage for react native, so the whole reason for the react-native example is to explain how to store data in it with the PouchDB RxStorage.

The entire PouchDb ecosystem is very outdated. For example, pouchdb-adapter-asyncstorage uses the deprecated async-storage (it was moved to community package). I had patched that package and fix that - the persistance works, but it doesn't fix sync. I tried to find working packages, but only the sqlite package with a bunch of crutches worked.

So, for this PR, we should deside what requirements are needed:

  • persistence?
  • sync throught http?
  • ... ?

@msotnikov msotnikov requested a review from pubkey July 28, 2022 13:18
@pubkey
Copy link
Owner

pubkey commented Jul 28, 2022

Yes both is needed. The reason for making the react native example was to show that RxDB can work with the react-native SQLite storage.
We can switch from couchdb-sync to the rxdb replication of course.

I assume this PR is made in context of the premium tasks, is that correct?

@msotnikov
Copy link
Contributor Author

msotnikov commented Aug 8, 2022

Yes both is needed. The reason for making the react native example was to show that RxDB can work with the react-native SQLite storage. We can switch from couchdb-sync to the rxdb replication of course.

I assume this PR is made in context of the premium tasks, is that correct?

Previous version doesn't use sqlite, it uses asyncstorage.

I have some success with custom version of pouchdb-adapter-react-native-sqlite in my project. But yes, I want to try your version of sqlite connector because, as I mention before, the whole pouchdb ecosystem is outdated.

I'll try to change the adapter to sqlite in this example project.

# Conflicts:
#	examples/react-native/App.js
#	examples/react-native/initializeDb.js
#	examples/react-native/package.json
@msotnikov
Copy link
Contributor Author

msotnikov commented Sep 1, 2022

Recent changes:

  1. Dependencies updated to current versions - expo@46.0.9, pouchdb@7.3.0, react@18.2.0, react-native@0.69.2
  2. Sync has fixed and tested with couchdb@3.2.2 instance (not included). Also, tested with pouchdb-server@4.2.2 - doesn't work.

There is no persistance functionality. Pouchdb need a crunches and Sqlite need a native modules to work. I don't want to deal with it for Expo. My suggestion is - rename this module to react-native-expo and leave as is in this PR. And create a new one with bare react-native.

@pubkey
Copy link
Owner

pubkey commented Sep 5, 2022

Strange error, I restarted the failed CI test.

@msotnikov
Copy link
Contributor Author

msotnikov commented Sep 5, 2022

I removed the cache settings. In any case, there is no package-lock file. And if I understood you correctly - you want to always have fresh packages.

The buggy jest and CI. I don't know how to fix it. Tests passed locally. Lost my patience. I'll be back later.

@msotnikov
Copy link
Contributor Author

So... Now we will test database.

@@ -484,28 +484,14 @@ jobs:
run: npm run test:build

react-native:
runs-on: ubuntu-20.04
runs-on: ubuntu-latest
Copy link
Owner

Choose a reason for hiding this comment

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

Please pin all dependencies, always. This goes for the npm modules and also for docker image versions etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@pubkey
Copy link
Owner

pubkey commented Sep 13, 2022

The code looks good to me.
What is the state of the sync now? I see it connects to 'http://admin:mysecret1@localhost:5984' but there is nothing started at this port.
As far as I remember the pouchdb sync did work before, or was it broken anyway?

@msotnikov
Copy link
Contributor Author

msotnikov commented Sep 13, 2022

The code looks good to me. What is the state of the sync now? I see it connects to 'http://admin:mysecret1@localhost:5984' but there is nothing started at this port. As far as I remember the pouchdb sync did work before, or was it broken anyway?

There was the similar code before^
const syncURL = 'http://localhost:10102/'; // Replace localhost with a public ip address!

I haven't seen this code works. I haven't seen any server side also.

@pubkey
Copy link
Owner

pubkey commented Sep 13, 2022

Ah, you are right, sorry havent seen that there never was a server.
So I can merge this now?

@msotnikov
Copy link
Contributor Author

Counting for it :-)

@pubkey pubkey merged commit 22f4c59 into pubkey:master Sep 13, 2022
@pubkey
Copy link
Owner

pubkey commented Sep 13, 2022

Merged, thank you for your work.
You can ping me in discord for the access to the premium plugins.

@msotnikov msotnikov deleted the fix-example-react-native-expo branch September 13, 2022 18:22
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 this pull request may close these issues.

None yet

2 participants