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 listener middleware Job class test file and initial usage test #1807

Merged
merged 4 commits into from
Dec 8, 2021

Conversation

markerikson
Copy link
Collaborator

@markerikson markerikson commented Dec 5, 2021

This PR:

  • Updates Job.pause() to include the existing _cancelPromise so that it bails out early if cancelation happens before the actual source promise resolves
  • Copies in the job.test.ts file from the original job library (and updates the assertions to work with Jest)
  • Adds the first of hopefully several usage scenario examples: a controllable long-polling loop based off of one I wrote years ago with sagas ( https://gist.github.com/markerikson/5203e71a69fa9dff203c9e27c3d84154 )

@codesandbox-ci
Copy link

codesandbox-ci bot commented Dec 5, 2021

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 3942f8f:

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

@netlify
Copy link

netlify bot commented Dec 5, 2021

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

🔨 Explore the source changes: 3942f8f

🔍 Inspect the deploy log: https://app.netlify.com/sites/redux-starter-kit-docs/deploys/61ac3768d0da65000808109f

😎 Browse the preview: https://deploy-preview-1807--redux-starter-kit-docs.netlify.app

@github-actions
Copy link

github-actions bot commented Dec 5, 2021

size-limit report 📦

Path Size
1. entry point: @reduxjs/toolkit (cjs.production.min.js) 12.23 KB (0%)
1. entry point: @reduxjs/toolkit (esm.js) 10.22 KB (0%)
1. entry point: @reduxjs/toolkit/query (cjs.production.min.js) 20.35 KB (0%)
1. entry point: @reduxjs/toolkit/query (esm.js) 17.37 KB (0%)
1. entry point: @reduxjs/toolkit/query/react (cjs.production.min.js) 22.22 KB (0%)
1. entry point: @reduxjs/toolkit/query/react (esm.js) 19.82 KB (0%)
2. entry point: @reduxjs/toolkit (without dependencies) (cjs.production.min.js) 5.56 KB (0%)
2. entry point: @reduxjs/toolkit (without dependencies) (esm.js) 5.53 KB (0%)
2. entry point: @reduxjs/toolkit/query (without dependencies) (cjs.production.min.js) 9.2 KB (0%)
2. entry point: @reduxjs/toolkit/query (without dependencies) (esm.js) 9.58 KB (0%)
2. entry point: @reduxjs/toolkit/query/react (without dependencies) (cjs.production.min.js) 2.37 KB (0%)
2. entry point: @reduxjs/toolkit/query/react (without dependencies) (esm.js) 2.21 KB (0%)
3. createSlice (esm.js) 5.16 KB (0%)
3. createEntityAdapter (esm.js) 5.83 KB (0%)
3. configureStore (esm.js) 5.83 KB (0%)
3. createApi (esm.js) 15.66 KB (0%)
3. createApi (react) (esm.js) 18.05 KB (0%)
3. fetchBaseQuery (esm.js) 10.93 KB (0%)
3. setupListeners (esm.js) 9.8 KB (0%)
3. ApiProvider (esm.js) 16.99 KB (0%)

@FaberVitale
Copy link
Contributor

FaberVitale commented Dec 7, 2021

I read your tweet about dropping jobs and using AbortController instead.

I think it is doable moreover I think we could play a bit with async generators:
complex logic can be achieved using them.


I believe we should drop when too:
redux-saga and redux-observable run after the reducers for good reasons.
I cannot think of any use case that when:before exclusively covers because we already provide getOriginalState.

We are just increasing the cognitive load for no apparent benefit.
I have personally worked with redux-saga and redux-observable in production apps and I find the "when" option more confusing than helpful.

I'm going to write a RFC in the other thread and a PR to drop this option.
what do you think?

@markerikson
Copy link
Collaborator Author

markerikson commented Dec 7, 2021

Hmm. I may not be fully understanding what you're suggesting about "async generators". I would prefer that this middleware not use generator functions, specifically because that's basically just reimplementing redux-saga at that point. (Obviously dealing with cancelation would be a whole lot easier if we were using generators, but the point is this is supposed to be a simpler alternative.) Are you saying that using async generators inside the listener could maybe act as a replacement for "jobs"? If so, what would that look like?

I think Lenz's concerns with jobs atm are a combination of bundle size and API size. Right now, dist/esm/index.js is about 6K minified. Half of that is the Job class, and about 2.5K is the middleware itself. I think we could maybe shave off 1-2K if we dropped Job and used a simpler implementation based around AbortController for the take/condition/cancelPrevious behavior, but I'm not sure what the final savings would be.

I agree that there probably aren't as many reasons to run the listeners before the dispatched action. It would be a good idea to go review #237 and #547 to see why we decided to add that in the first place. I think part of it was because Lenz originally had that listenerApi.preventDefault() function that could be used to cancel the action from reaching the reducers, but we removed that on the grounds that it was also confusing.

If you can do some research on why we added when and some brainstorming as to whether there are still any use cases where we might want to run the listeners 'beforeReducer', that would be very helpful.

Also please feel free to try out something with AbortControllers that drops the Job class. I'm not saying we'll merge it right away :) but it would be good to compare implementations.

I did see an interesting implementation at https://gist.github.com/andrewcourtice/ef1b8f14935b409cfe94901558ba5594 that looked like it might be a good starting point for something like that.

@markerikson markerikson changed the title Add listener middleware usage scenario tests Add listener middleware Job class test file and initial usage test Dec 8, 2021
@markerikson markerikson merged commit cecda16 into master Dec 8, 2021
@markerikson markerikson deleted the feature/middleware-job-tests branch December 8, 2021 00:05
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