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

RxDocument.remove() throws on removal() when deleted #830

Closed
rafamel opened this issue Sep 29, 2018 · 4 comments
Closed

RxDocument.remove() throws on removal() when deleted #830

rafamel opened this issue Sep 29, 2018 · 4 comments

Comments

@rafamel
Copy link
Contributor

rafamel commented Sep 29, 2018

await doc.remove();
console.log(doc.deleted); // true
await doc.remove();

Rx.Document.remove() is throwing instead of promise-reject. From rx-document.js:

    // ...
    remove() {
        if (this.deleted) {
            throw RxError.newRxError('DOC13', {
                document: this,
                id: this.primary
            });
        }
        // ...
      }
    //...  

I haven't sent a PR as I'm thinking this might be intentional, but since it is an async method, I do think it should reject rather than throw.

@pubkey
Copy link
Owner

pubkey commented Sep 29, 2018

You are right. It schould return a rejected promise.

@rafamel
Copy link
Contributor Author

rafamel commented Sep 29, 2018

Sending a PR your way tonight!

rafamel added a commit to rafamel/rxdb that referenced this issue Sep 30, 2018
@pubkey pubkey closed this as completed in 79c257b Jan 7, 2019
@pubkey
Copy link
Owner

pubkey commented Jan 7, 2019

Hi @rafamel I added the change so calling remove() will return a rejected promise.
I still do not think that we should wrap all things in async functions because it increases the build size too much, which is already a critical factor when using RxDB. In the future, like in a year or so, we can stop shipping transpiled code and then switch to plain async/await.

@rafamel
Copy link
Contributor Author

rafamel commented Jan 14, 2019

I agree with you @pubkey. Essentially, the rationale behind this was pure laziness. Aka, an async function will always return properly, and checking the whole codebase against it wasn't a sensible option time-wise. Ultimately, the only guarantee needs to be against the public api, so my approach would be to just check the functions susceptible of public use and ensure that they promise reject instead of throwing. It was quite a while ago, but I do remember I found a few public methods that didn't return properly (just like remove()); it might be sensible to check for promise returning functions and fix those that are a part of the public api.

Thanks for the work, @pubkey !
Rafa.

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

No branches or pull requests

2 participants