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 return type of database extension store method #715

Merged

Conversation

haines
Copy link
Contributor

@haines haines commented Oct 5, 2023

This PR changes the type of the database extension's store method from

(data: storePayload) => void

to

(data: storePayload) => Promise<void>

to make it clear that the function is expected to be asynchronous and will be awaited in the onStoreDocument hook.

Also, instead of returning the promise directly from the extensions onStoreDocument, it is now awaited to ensure that the call stack on any error is correct.

Signed-off-by: Andrew Haines <andrew@haines.org.nz>
@janthurau janthurau merged commit 0912c4c into ueberdosis:main Oct 9, 2023
3 checks passed
@GermanJablo
Copy link
Collaborator

@janthurau and @haines

Could there be something wrong with this PR? if I do

store: (data) => { return; }, I get the error:

Type 'void' is not assignable to type 'Promise'

and if I do:

store: async (data) => { return Promise.resolve();}, I get the error:

Promise-returning function provided to property where a void return was expected. (eslint@typescript-eslint/no-misused-promises)

@haines
Copy link
Contributor Author

haines commented Dec 12, 2023

Using an async function is correct. I think the linter is complaining because you are returning something unnecessarily. Try this:

store: async (data) => { await Promise.resolve(); }

Technically for a no-op I think you could just do

store: async (data) => { }

but the linter will probably complain about that too.

@GermanJablo
Copy link
Collaborator

Yeah, I tried those two too and a few other combinations. The linter always complains:

store: async (data) => void {},
store: async (data) => {},
store: async (data) => await Promise.resolve(),
store: async (data) => { await Promise.resolve(); }
store: async (data) => { return Promise.resolve(); }

I don't understand what's going on here. Can you reproduce the error if you enable eslint@typescript-eslint/no-misused-promises?

@haines
Copy link
Contributor Author

haines commented Dec 13, 2023

I'm afraid I'm travelling at the moment so I can't check if I can reproduce for a few days.

I would maybe try explicitly providing the return type like

store: async (data): Promise<void> => { await Promise.resolve(); }

@haines haines deleted the database-extension-store-returns-promise branch December 20, 2023 07:38
TalhaASiddiqi pushed a commit to educative/hocuspocus that referenced this pull request Feb 1, 2024
Signed-off-by: Andrew Haines <andrew@haines.org.nz>
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

3 participants