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: allow async functions in beforeSend and afterSend #166

Closed
wants to merge 3 commits into from
Closed

fix: allow async functions in beforeSend and afterSend #166

wants to merge 3 commits into from

Conversation

Bobby-McBobface
Copy link
Contributor

This patch allows async functions to be in beforeSend and afterSend to be executed correctly. Sync functions aren't affected because await works for sync functions too.

@ganigeorgiev
Copy link
Member

Could you elaborate what is your use case for this?

@Bobby-McBobface
Copy link
Contributor Author

pb.beforeSend = async (url, option) => { 
    // Do async stuff here. 
    await ...
} 

My use case is to run await a fetch in the beforeSend, but this would allow many more async usecases, without much code change.

@ganigeorgiev
Copy link
Member

ganigeorgiev commented Mar 16, 2023

I understand what it does, but I don't know why you would want to run fetch in a beforeSend function for example.

In any case, I guess we can allow it but we also need to update the beforeSend TS return signature (maybe with union type?) - https://github.com/pocketbase/js-sdk/blob/master/src/Client.ts#L42

@ganigeorgiev
Copy link
Member

We also will need unit tests for the async hook behavior in https://github.com/pocketbase/js-sdk/blob/master/tests/Client.spec.ts#L197-L254

@Bobby-McBobface
Copy link
Contributor Author

Bobby-McBobface commented Mar 16, 2023

I have added passing tests that fail without the change.

but I don't know why you would want to run fetch in a beforeSend function for example.

My use case is a little bit dumb, but basically my hosting provider goes to sleep after a while of inactivity, and I have a beforeSend hook to check if it's alive and to start it if it's not. However, I believe this is a small change with little maintenance needed, and it allows for more usecases than my own.

@Bobby-McBobface
Copy link
Contributor Author

Bobby-McBobface commented Mar 16, 2023

Wait, don't merge yet, it seems like I ran a linter on unrelated parts of the code
It's now ready for a review.

@ganigeorgiev
Copy link
Member

ganigeorgiev commented Mar 16, 2023

I've squash merged it locally with some minor changes in relation to the tests to ensure that the hooks Promise is actually resolved.

I will push sometime later today v0.13.0-rc release (the stable release will be available most likely together with the PocketBase v0.14.0 release in the next couple weeks).

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.

2 participants