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

Remove local docs when remove is called on the database #3397

Conversation

special-character
Copy link

@special-character special-character commented Sep 17, 2021

This PR contains:

  • A BUGFIX

Describe the problem you have without this PR

fixes: #3396

Todos

  • Tests
  • Fixing the bug

storage.createKeyObjectStorageInstance(databaseName, collectionName + '-local', {})
]);

await Promise.all([instance.remove(), localInstance.remove()]);
Copy link
Author

Choose a reason for hiding this comment

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

The "fix" was to make sure the local instance was removed. If we take out localInstance.remove(), the test fails.

}
PouchDB.plugin(require('pouchdb-adapter-memory'));
const db = await createRxDatabase<{ human: RxCollection<schemaObjects.HumanDocumentType> }>({
name: 'humandatabase',
Copy link
Author

@special-character special-character Sep 17, 2021

Choose a reason for hiding this comment

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

This is duplicated code and shouldn't really be this way. This was an easy way to quickly create the database without a different random name so that I could prove the local docs aren't removed.

I am betting the other tests that create random database names might break if they had a non-random database name.

@special-character special-character changed the title Add breaking test to confirm local doc is not deleted on removeRxDatabase Remove local docs when remove is called on the database Sep 17, 2021
@pubkey
Copy link
Owner

pubkey commented Sep 20, 2021

Hi @special-character
I fixed #3319 with this commit.
Can you check if that also solves your problem?

@special-character
Copy link
Author

special-character commented Sep 20, 2021

Thanks @pubkey, it looks like it should but I can test it out. Is this deployed in 10.0.3? I was certain when I upgraded I got the latest version I saw on npm so I am confused about how I didn't get this.

edit: oh, I see the commit a few hours ago. I can grab them from the dist and try it out

@special-character
Copy link
Author

Looks like the dist isn't actually built out from the src on master. Is there something I should do to build that?

@pubkey
Copy link
Owner

pubkey commented Sep 20, 2021

I started the build job when this is finished the dist folder should be up to date.

@special-character
Copy link
Author

@pubkey I got a chance to pull the dist folder into my project and try it out. As far as I can tell, it appears to work

@pubkey
Copy link
Owner

pubkey commented Sep 20, 2021

Thank you for testing. I will release a new version tomorrow.
By the way you can also use the latest non-released rxdb version by using the commit hash in your package.json. see here

@special-character
Copy link
Author

Going to close this because it was already done in the upstream repo.

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.

Some local documents don't seem to get deleted on remove
2 participants