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

correctly infer dispatch from provided middlewares #304

Merged
merged 8 commits into from
Jan 16, 2020

Conversation

phryneas
Copy link
Member

@phryneas phryneas commented Jan 3, 2020

So this is my first attempt at solving #289.

Still a bit rough around the edges, but that's it for today.

  • feature itself is working
  • type documentation
  • skip tests using as const in TS 3.3
  • update typescript usage documentation

@netlify
Copy link

netlify bot commented Jan 3, 2020

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

Built with commit 771d393

https://deploy-preview-304--redux-starter-kit-docs.netlify.com

export type DefaultMiddlewareResult<
S,
O extends GetDefaultMiddlewareOptions
> = Unshift<
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

whimper :)

Copy link
Member Author

@phryneas phryneas Jan 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, wanna REALLY hurt? Take a look at the implementation of Unshift ;)

Here, I had the choice between

  • make getDefaultMiddleware always return [ThunkMiddleware], which is untrue (but would make stuff soooo much simpler!)
  • create a type with one case for every possible combination of thunk, immutableCheck and serializableCheck (I had this in for a while)
  • this one: create the tuple of length 0 to 3, and one check per possible configuration option. The only way to make a tuple longer in TS is currently to prepend values, to I'm starting with the empty tuple [] and prepending the three middlewares depending on their configuration value from the innermost to the outermost Unshift. At least it doesn't repeat itself too much 🤷‍♂️

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So now what happens if we add another entry to getDefaultMiddleware?

Copy link
Member Author

@phryneas phryneas Jan 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This way: another level of Unshift, essentially two more lines. I might still extract the types in there out, that will make it more readable.

The way I had it before? Go from 3! cases to 4! cases.

Problem is: now that the Dispatch type relies on the value of middleware, getDefaultMiddleware needs to return it's value accurately.

Although I can think of a fourth way right now: instead of the tuple, it could also just return an array with specific values, as right now the order of middlewares is not taken into account.

That might be simpler to type - it was just not possible with an earlier implementation of DispatchForMiddlewares. I'll investigate that tomorrow.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, it worked out - that's much simpler now.

@codesandbox-ci
Copy link

codesandbox-ci bot commented Jan 4, 2020

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 771d393:

Sandbox Source
optimistic-galois-xsdbc Configuration
dank-https-c0pm2 Configuration

@phryneas phryneas marked this pull request as ready for review January 10, 2020 12:35
@phryneas
Copy link
Member Author

Okay, documentation is updated, too. Could you please give this a look @markerikson ? :)

# Conflicts:
#	etc/redux-toolkit.api.md
#	src/getDefaultMiddleware.ts
@markerikson
Copy link
Collaborator

Awright, putting this in.

@markerikson markerikson merged commit 775da05 into reduxjs:master Jan 16, 2020
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.

None yet

2 participants