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

Use a branded Promise type for always-resolved Promises #4101

Closed
JoshuaKGoldberg opened this issue Jan 20, 2024 · 2 comments · Fixed by #4102
Closed

Use a branded Promise type for always-resolved Promises #4101

JoshuaKGoldberg opened this issue Jan 20, 2024 · 2 comments · Fixed by #4102

Comments

@JoshuaKGoldberg
Copy link
Contributor

JoshuaKGoldberg commented Jan 20, 2024

Coming over from typescript-eslint/typescript-eslint#7008 (comment): there is a known annoyance with the @typescript-eslint/no-floating-promises lint rule that it flags the Promises created by createAsyncThunk as "floating" if not handled, despite those Promises being known "safe" ones that never resolve. There's no way for the lint rule to know that these Promises are safe.

That issue tracks adding an allow option to the lint rule that would let users allowlist these Promises. But in order to help with RTK, the known safe Promises would need some kind of specific type that can be added to the rule's allowlist. +1 to @phryneas's suggestion:

So, it would be nice if we could mark those promises with some kind of branded type to them to signal to eslint that these promises can be handled, but don't need to.

We're up for adding in allow on our end if you're up for adding that branded type here! Is that something you're up for? Even if this issue is marked as blocked on us implementing the allow option, as long as that's the only blocker we're good to go.

I'd be happy to contribute the actual code change to RTK myself if you think it's doable for a first-timer. 😄

Edit: to be clear, this is roughly what users would be able to specify in their ESLint config once typescript-eslint/typescript-eslint#7008 is resolved (pending naming bikeshedding):

"@typescript-eslint/no-floating-promises": ["error", {
  "allowForKnownSafePromises": [
    { "from": "package", "name": "SafePromise", "package": "@reduxjs/toolkit"
  ]
}]
@markerikson
Copy link
Collaborator

Sure, happy to take that PR :)

I will say that the promise / error handling logic inside createAsyncThunk is fairly gnarly, but not impossible to deal with.

@JoshuaKGoldberg
Copy link
Contributor Author

Btw I just edited the OP to make it clear what the changes in a user's ESLint config would be. Only realized after the fact I was never clear on that. Sorry 🙂

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 a pull request may close this issue.

2 participants