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

Add error reporting for attempted async selector set #777

Closed

Conversation

drarmstr
Copy link
Contributor

@drarmstr drarmstr commented Dec 1, 2020

Summary: Recoil does not support async selector sets. However, as it does support async gets many users expect async sets. Flow reports a static error if an async function is used, TypeScript should as well. Though users may not have or notice this error. This diff throws an error if callbacks in the set are attempted to be used asynchronously. However, those errors may not be caughts and be lost. So, we also detect if an async function was used by checking if the user's set returns a Promise and throws an error for that as well.

Differential Revision: D25250686

Summary: Recoil does not support async selector sets.  However, as it does support async gets many users expect async sets.  Flow reports a static error if an `async` function is used, TypeScript should as well.  Though users may not have or notice this error.  This diff throws an error if callbacks in the `set` are attempted to be used asynchronously.  However, those errors may not be caughts and be lost.  So, we also detect if an `async` function was used by checking if the user's set returns a `Promise` and throws an error for that as well.

Differential Revision: D25250686

fbshipit-source-id: 113398d9ca09ff4ff47f681125cc91967750a0ab
@facebook-github-bot facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. fb-exported labels Dec 1, 2020
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D25250686

@jjalonso
Copy link

jjalonso commented Dec 2, 2020

Will async sets on selector will never be implemented?
its quite unfair that it is for gets only, some people like me need to derivate data using promise, one example is this.

Imagine I have two kind of events that im subscribed on externally, channelAdded, channelRemoved, on channelAdded and channelRemoved i get as argument a channel instance from an 3rd party lib, and need to call some methods that return promises, like getMembers() etc, in order to serialize,

Then we endup having a bidirectional selector get/set where the get return alls channels and the set have a switch case on the argument to evaluate value.type 'ADDED' or 'REMOVED' in order to do something with value.payload but just the added need to serialize. Ideally we should need to have the derivate data logic inside the setter and not doing out previously the set call,

currently with the current implementation that doesnt support async set, we need to serialise out and send to the set the channel serialize in case of added and the channel without serialize in case of remove.

@drarmstr
Copy link
Contributor Author

drarmstr commented Dec 2, 2020

@jjalonso - Continuing discussion in #762

@facebook-github-bot
Copy link
Contributor

This pull request has been merged in 73345f5.

@drarmstr drarmstr deleted the export-D25250686 branch December 16, 2020 20:06
AlexGuz23 pushed a commit to AlexGuz23/Recoil that referenced this pull request Nov 3, 2022
Summary:
Pull Request resolved: facebookexperimental/Recoil#777

Recoil does not support async selector sets.  However, as it does support async gets many users expect async sets.  Flow reports a static error if an `async` function is used, TypeScript should as well.  Though users may not have or notice this error.  This diff throws an error if callbacks in the `set` are attempted to be used asynchronously.  However, those errors may not be caughts and be lost.  So, we also detect if an `async` function was used by checking if the user's set returns a `Promise` and throws an error for that as well.

Reviewed By: davidmccabe

Differential Revision: D25250686

fbshipit-source-id: 93c529f09734142cdd04ea0eda7097f814b120cb
snipershooter0701 pushed a commit to snipershooter0701/Recoil that referenced this pull request Mar 5, 2023
Summary:
Pull Request resolved: facebookexperimental/Recoil#777

Recoil does not support async selector sets.  However, as it does support async gets many users expect async sets.  Flow reports a static error if an `async` function is used, TypeScript should as well.  Though users may not have or notice this error.  This diff throws an error if callbacks in the `set` are attempted to be used asynchronously.  However, those errors may not be caughts and be lost.  So, we also detect if an `async` function was used by checking if the user's set returns a `Promise` and throws an error for that as well.

Reviewed By: davidmccabe

Differential Revision: D25250686

fbshipit-source-id: 93c529f09734142cdd04ea0eda7097f814b120cb
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. fb-exported Merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants