redux icon indicating copy to clipboard operation
redux copied to clipboard

Fix type of next parameter in StoreEnhancer type

Open Methuselah96 opened this issue 5 years ago • 9 comments


name: "Fix type of next parameter in StoreEnhancer type" about: Fix type of next parameter in StoreEnhancer type

PR Type

Does this PR add a new feature, or fix a bug?

Fix a bug. Resolves https://github.com/reduxjs/redux/issues/3768.

Why should this PR be included?

Without this PR the type for next in StoreEnhancer is incorrect. It assumes that next will create a store with the current enhancers Ext and StateExt already applied to store. Instead, next returns a store with NextExt and NextStateExt generic parameters since the enhancer should work when composed with any other enhancer. It then expects the StoreEnhancer to return a new store creator that will create a store with Ext and NextExt, and StateExt and NextStateExt.

Note that I had to remove the ExtendState type in order to make this work. This type was introduced in https://github.com/reduxjs/redux/pull/3524, but I wasn't able to get the types working while using ExtendState. I think the types are more clear without it anyway.

Checklist

  • [x] Have you added an explanation of what your changes do and why you'd like us to include them?
  • [x] Is there an existing issue for this PR?
    • https://github.com/reduxjs/redux/issues/3768
  • [x] Have the files been linted and formatted?
  • [x] Have the docs been updated to match the changes in the PR?
  • [x] Have the tests been updated to match the changes in the PR?
  • [x] Have you run the tests locally to confirm they pass?

Bug Fixes

What is the current behavior, and the steps to reproduce the issue?

Steps to reproduce are in https://github.com/reduxjs/redux/issues/3768. There's also a new test for checking the types on stores created from composed enhancers. This test fails in master.

What is the expected behavior?

The types should allow for a generic NextExt and NextStateExt.

How does this PR fix the problem?

It adds those generics.

Methuselah96 avatar May 13 '20 17:05 Methuselah96

Deploy preview for redux-docs ready!

Built with commit c5863d52aaa2315c876dcefc0a39ce95e201a128

https://deploy-preview-3776--redux-docs.netlify.app

netlify[bot] avatar May 13 '20 17:05 netlify[bot]

Deploy preview for redux-docs ready!

Built with commit 92cf2a2429ab87f1118e9f33cc43bcf731893743

https://deploy-preview-3776--redux-docs.netlify.app

netlify[bot] avatar May 13 '20 17:05 netlify[bot]

Can you fix the conflicts?

samarmohan avatar May 17 '21 23:05 samarmohan

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 5a084a62fda9ebf39ffa969458d9950b4f051688:

Sandbox Source
Vanilla Configuration
Vanilla Typescript Configuration

codesandbox-ci[bot] avatar May 18 '21 01:05 codesandbox-ci[bot]

I'll try to circle back and actually look at this in the near future. My main observation atm is that this is gonna go onto master, and we have no active plans to actually release the code in master at this point. It's the 4.x branch that matters for the foreseeable future.

markerikson avatar Apr 26 '22 15:04 markerikson

@markerikson No problem, don't mean to guilt you into looking into this but the old types are objectively wrong and so I'd love to get this merged. I'd be happy to create a separate PR to target 4.x.

Methuselah96 avatar Apr 26 '22 15:04 Methuselah96

@markerikson Also https://github.com/reduxjs/redux/pull/3772 really needs to get merged at some point as well. 😉

Methuselah96 avatar Apr 26 '22 15:04 Methuselah96

Let me know when you're ready to look at either one and I can resolve merge conflicts.

Methuselah96 avatar Apr 26 '22 15:04 Methuselah96

I'd like to try to do some general Redux org issue triage and cleanup over the next couple weeks, so yeah, getting this knocked out would be high on the priority list.

markerikson avatar Apr 26 '22 15:04 markerikson