Skip to content

upgrade pouchdb to 7.3.0 #3807

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.

Sign up for GitHub

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

Jump to bottom
Merged
pubkey merged 1 commit into from
May 24, 2022
Merged

upgrade pouchdb to 7.3.0 #3807

pubkey merged 1 commit into from
May 24, 2022

Conversation

@cetsupport
Copy link

@cetsupport cetsupport commented May 21, 2022 •
edited
Loading

This PR contains:

  • IMPROVED TESTS

Describe the problem you have without this PR

CI/CD test fail when upgrade to pouchDB 7.3.0

Fix the issue upgrade to pouchdb 7.3.0

@pubkey
Copy link
Owner

pubkey commented May 23, 2022

So was this problem already fixed or did the master branch get changed so that we do not detect the error?

@cetsupport
Copy link
Author

cetsupport commented May 24, 2022

This problem was fixed by this PR. The reason is changed the selector from $gt to $ne to avoid race condition in pouchdb.

@pubkey pubkey merged commit cbe4972 into pubkey:master May 24, 2022
pubkey added a commit that referenced this pull request May 24, 2022
@pubkey
ADD(changelog) #3807
65e62ed
@pubkey pubkey mentioned this pull request May 24, 2022
Closed
4 tasks
@pubkey
Copy link
Owner

pubkey commented May 24, 2022 •
edited
Loading

Ah, I see.
Please ping me in gitter so I can send you the license agreement for the premium plugins.

@cetsupport

@PaulMest
Copy link
Contributor

PaulMest commented May 24, 2022

I feel like this "fix" is a dangerous approach. There was a race condition when using pouchdb 7.3.0 in-memory-adapter and changing the tests to just ignore the race condition doesn't fix the problem (outlined in #3763 (comment).). If you add more tests, this will likely come up again.

I'd suggest either:

  1. Reverting this fix because either PouchDB in-memory-adapter 7.3.0 isn't isolated properly or something about the invocation of the in-memory-adapter in RxDB isn't configured properly.
    ... or ...
  2. There should be at least an additional test that covers the race condition directly and shared with the PouchDB team.

@pubkey
Copy link
Owner

pubkey commented May 24, 2022

I also thought about this before I merged the PR.
I think having a race condition on pouchdb 7.3.0 is still better then using 7.2.2.

Many other things are broken in 7.2.2 where queries do not work or npm install fails.

Having a query withi $gt: null is something that should not happen in production because it is not clear if a string/number type can be greater-then compared with null. In the future I do not want to allow mixed types in RxDB, see here

Yes we should make a PR with a failing test in the pouchdb repo. But in the past this was not worth the time because pouchdb uses the stale bot which just closes them again.
Check my already opened and ignored issues: https://github.com/pouchdb/pouchdb/issues?q=is%3Aissue+pubkey
So it would be cool if someone else could do that.

@PaulMest
Copy link
Contributor

PaulMest commented May 25, 2022

Ahh... thanks for the context. I didn't realize there were known issues and warts with staying on 7.2.2. I guess 7.3.0 with this race condition is the lesser of two evils? I'm leaving on vacation soon and have a million things to do, but I'll see if I can create a repro and post it as an open PR.

@pubkey
Copy link
Owner

pubkey commented May 25, 2022

I guess 7.3.0 with this race condition is the lesser of two evils

Yes, at least I think so.
Would be cool if you find the time to report this at PouchDB.

@PaulMest
Copy link
Contributor

PaulMest commented May 25, 2022

Ok, here is the repro case #3813. I'll also report this over in PouchDB.

PaulMest added a commit to PaulMest/pouchdb that referenced this pull request May 25, 2022
@PaulMest
repro: Race condition for pouchdb-memory-adapter
dfd0e49 
This is a simple repro for a problem discovered in PouchDB 7.3.0. This was originally discovered in RxDB. Discussion here: pubkey/rxdb#3807.

```
  134 passing (2s)
  3 pending
  1 failing

  1) test.memory-adapter.js Race condition initially discovered with PouchDB in-memory-adapter 7.3.0:
     Error: Timeout of 2000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves.
      at listOnTimeout (node:internal/timers:559:17)
      at processTimers (node:internal/timers:502:7)
```
@PaulMest PaulMest mentioned this pull request May 25, 2022
Merged
pubkey added a commit that referenced this pull request Jun 7, 2022
@pubkey
FIX merge of #3807 was ignored
dab2fd9
PaulMest added a commit to PaulMest/pouchdb that referenced this pull request Dec 7, 2022
@PaulMest
repro: Race condition for pouchdb-memory-adapter
d890645 
This is a simple repro for a problem discovered in PouchDB 7.3.0. This was originally discovered in RxDB. Discussion here: pubkey/rxdb#3807.

```
  134 passing (2s)
  3 pending
  1 failing

  1) test.memory-adapter.js Race condition initially discovered with PouchDB in-memory-adapter 7.3.0:
     Error: Timeout of 2000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves.
      at listOnTimeout (node:internal/timers:559:17)
      at processTimers (node:internal/timers:502:7)
```
@PaulMest
Copy link
Contributor

PaulMest commented Jan 3, 2023

FYI, PouchDB accepted the fix proposed and has released it as part of PouchDB 8.0.0.

@pubkey
Copy link
Owner

pubkey commented Jan 3, 2023

Yes I know. But the pouchdb storage is deprecated anyways, so I will not update PouchDB.

@meabed
Copy link

meabed commented Jan 3, 2023 •
edited
Loading

I know it's another big effort - but I alot of folks would appreciate keep in pouchdb - I think most of react native setup uses pouch db, any change to support pouchdb if it worth it.

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

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@cetsupport @pubkey @PaulMest @meabed