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: abort controller fallback #2446

Closed

Conversation

JulienKode
Copy link
Contributor

@JulienKode JulienKode commented Jun 23, 2022

Description

Inside createAsyncThunk currently RTK have a fallback in case AbortController is not declared.

typeof AbortController !== 'undefined'

This seems not the case for the listenerMiddleware

const childAbortController = new AbortController()

How to reproduce the issue ?

  • Create a next.js project with node 14
  • use listener middleware on server side
  • see the reference error of AbortController

@codesandbox-ci
Copy link

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 242d99f:

Sandbox Source
Vanilla Configuration
Vanilla Typescript Configuration
rsk-github-issues-example Configuration
@examples-query-react/basic Configuration
@examples-query-react/advanced Configuration
reduxjs/redux-toolkit Configuration

@netlify
Copy link

netlify bot commented Jun 23, 2022

Deploy Preview for redux-starter-kit-docs ready!

Name Link
🔨 Latest commit 242d99f
🔍 Latest deploy log https://app.netlify.com/sites/redux-starter-kit-docs/deploys/62b4bbb4a1abe70008b738b2
😎 Deploy Preview https://deploy-preview-2446--redux-starter-kit-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@phryneas
Copy link
Member

Hi, thanks for the PR!

At this point I am thinking we should rather remove that polyfill: IE has dropped and there is no more desktop browser supporting it. In node environments, that polyfill can cause more harm than good, since some fetch polyfills only work with very specific AbortController polyfills.

So my suggestion would be that we keep it for the time being and remove that polyfill in the 2.0 major. @markerikson what are your thoughts?

@JulienKode
Copy link
Contributor Author

Hi @phryneas

Thanks for your feedback on this,
Seems a good idea

What will be the plan for the v2, should AbortController be mandatory to use RTK ?

@phryneas
Copy link
Member

phryneas commented Jun 23, 2022

I haven't thought about it in-depth, but given that you don't need a polyfill in the browser any more, node 16 ships with AbortController, and (in the case of older node versions) on the server installing a polyfill also doesn't consume any "over the wire data", I think it would be okay to say "we expect an AbortController implementation to be available".

@JulienKode
Copy link
Contributor Author

This makes sense, in the same way as fetch

@JulienKode
Copy link
Contributor Author

JulienKode commented Jun 23, 2022

Should I close this PR or it's still relevant for pre 2.0 version ?

@phryneas
Copy link
Member

Good question - could you close this, but open an issue in the other direction a la "remove abortcontroller" so we can label that as 2.0 and track it? :)

@JulienKode
Copy link
Contributor Author

Thanks I've created #2448

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