Handle the closing of dependent databases from the abstract adapter #8575
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
PromiseRejectionEvent is not available in Node.js. This fix allows real errors to be printed instead of 'ReferenceError: PromiseRejectionEvent is not defined'. Also see: https://developer.mozilla.org/en-US/docs/Web/API/PromiseRejectionEvent#browser_compatibility
…ely when multiple instances are used" This reverts commit 24157fc.
This reverts commit 9bf1310. Except from the .gitignore changes that we want to keep.
This was referenced Dec 13, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Context
This PR shows how we could align the code responsible to handle the closing of dependent databases with the existing code for database destroy.
All details available in: Inconsistent handling of dependent databases during close and destroy - #8574
Fixes #8574
Additional comments
Skipped tests
I had to skip 2 more tests in tests/integration/test.close.js because they were incompatible with the changes in this PR.
Note that this test file, introduced more than 5 years ago in 96a5dfc, contains a majority of tests labeled
FIXME... which have never been fixed since. I don't know the original author's intention, but it could be that they wanted to support future behaviors (like "destroy after close" on a database) but they were never implemented. I wonder if this test file should be discarded as it doesn't add much value... only confusion for PRs like this. Indeed, the tests I had to skip previously worked "accidentally" and not because of proper design to support them.As said in issue #8574, I am not an expert of PouchDB so other opinions are very welcome.
Test for original issue #7331
The investigations leading to this proposal started after a patch was merged to fix #7331 (more about that in #8574).
Note that no tests were required to merge this patch at the time.
I considered adding a test for this use case to avoid regressions, but I have to admit that I don't understand exactly what was needed at the time... and even if the scenario should be supported (deleting files from disk can "desynchronize" the contents of '_local/_pouch_dependentDbs'). Feel free to give me more context if something is missing in this regard.