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

Create new npm release #299

Closed
tony-scio opened this issue Sep 2, 2020 · 21 comments
Closed

Create new npm release #299

tony-scio opened this issue Sep 2, 2020 · 21 comments

Comments

@tony-scio
Copy link

Feature Request

There are a lot of good updates since the last npm release over 2 years ago. v2.3.0...master

It'd be really great if whoever has access could cut a new release.

@FernandoChu
Copy link

Second this, mainly the changes to the types definitions would have helped me a lot.

@eaglus
Copy link

eaglus commented Sep 10, 2020

@timdorr @gaearon @aikoven Please publish a latest version of this package. We use our private npm package based on latest master of redux-think, and dream to have a latest official version, and drop out private package

@gaearon
Copy link
Collaborator

gaearon commented Sep 11, 2020

@tony-scio Do you want to cut npm releases? I'm happy to add as an owner.

@timdorr timdorr closed this as completed Sep 11, 2020
@timdorr timdorr reopened this Sep 11, 2020
@markerikson
Copy link
Contributor

FWIW, the last time Tim and I chatted about this, he was concerned that the current types may not be sufficiently correct or something along those lines. Tim, can you clarify what those concerns are atm?

@timdorr
Copy link
Member

timdorr commented Sep 11, 2020

Yes, I'm primarily concerned that we're using a number of overloads to apply the thunk types, rather than preferring the middleware approach. This means that the Dispatch and bindActionCreators types get overridden if the module is included, even if it's not used.

This also creates a problem for using multiple middlewares on the same store, where they may get clobbered by this overload. I'm not sure of the priority order on overloads vs. directly applied types, but that uncertainty makes me wary.

For reference, here are the PRs with type changes since 2.3.0:

#216
#219
#224
#243
#255
#260
#263
#278

I have some regret about merging in #224 and #278, since they add the overloads. They also tie to the specific types we have in Redux 4.x, and will break under 5.0.

@tony-scio
Copy link
Author

@gaearon I'm probably not yet well qualified to be responsible for redux-thunk releases as I haven't developed in this library. I'm just a consumer of the library who is still very much looking forward to the next release.

@flymoondust
Copy link

I'm also looking forward the next release。My company has standard for picking third part library,Now for 2 years no new release, I may be forced to pick alternative library, but I don't know any other library.

@timdorr
Copy link
Member

timdorr commented Oct 22, 2020

Just keep in mind the only thing that has changed in that time is the build system and the TypeScript types. The code of the actual middleware hasn't changed in 4 years, and that was only to add extraArgument.

@tony-scio
Copy link
Author

@timdorr apologies for the naive question, but why not make a release branch and, on it, back out the 2 commits you mentioned that you regret? One of the other type improvements would have saved me a prod bug. Seems unfortunate to hold them back from users.

@gregtatum
Copy link

Just chiming in to say that I've put lots of hours over the years into trying to type thunks in both Flow types and TypeScript, so having some kind of type-safe solution that is type-correct for connected components, even if not ergonomic would be a huge windfall. I'm more concerned about catching bugs, rather than having an ergonomic API. This typing doesn't appear to be a solved problem with any kind of official recommendation, even if it's a "this is not great, but it works".

That said, thanks for the maintenance work, I find redux-thunk essential for the apps I maintain.

@GLObus303
Copy link

Hey, any plans on releasing the fixes?

@reduxjs reduxjs deleted a comment from flymoondust Mar 25, 2021
@Philipp91
Copy link
Contributor

Yes, I'm primarily concerned that we're using a number of overloads

Fair. microsoft/TypeScript#14107 is annoying, I contributed a workaround here, but the root problem (the overloads) remains. Elsewhere they recommend:

you should never write a series of overloads which have an equivalent representation in union types.

Still, as it's been three years, I'm in favor of a release or some move in some direction. I currently use the latest release "redux-thunk": "^2.3.0",, and I'm getting plenty of TypeScript errors due to #248 unless I manually patch the workaround.

@markerikson
Copy link
Contributor

As mentioned in that other thread, using our official Redux Toolkit package (which integrates thunks into configureStore) should result in correct types behavior, and you should be using RTK anyway:

https://redux-toolkit.js.org/api/configureStore

@Philipp91
Copy link
Contributor

Here's how to reproduce the problem with Redux Toolkit (and applying the aforementioned workaround fixes it here too):

// type-tests/files/createMixedThunk.typetest.ts

import {AnyAction, PayloadAction} from '@reduxjs/toolkit'
import {ThunkAction, ThunkDispatch} from 'redux-thunk'

const defaultDispatch = (() => {
}) as ThunkDispatch<{}, any, AnyAction>
type AppAction = PayloadAction<number> | ThunkAction<void, {}, any, AnyAction>;

;(async function () {
    const createAction = (id: number): PayloadAction<number> => ({type: '', payload: id});
    const createAsync = (id: number): ThunkAction<void, {}, any, AnyAction> => async (dispatch) => {
        dispatch(createAction(id));
    };
    const createMixed = (choose: boolean, id: number) => choose ? createAction(id) : createAsync(id);

    defaultDispatch(createAction(42)); // Works fine
    defaultDispatch(createAsync(42)); // Works fine
    const mixed: AppAction = createMixed(true, 42);
    defaultDispatch(mixed);
    //             ^^^^^^^ -- Argument types do not match parameters
})();

Basically the mixed: AppAction variable is either a plain old action, or it's a thunk. The reason we don't know and have to use a type union is because of some application logic (not part of the example code above) that passes the action around (which is why the AppAction type exists to begin with, though the error message is the same without that type). Imagine some helper functions/classes with signatures that accept or produce AppAction instances, so there's really no way to know which of the two action types it is.

@Philipp91
Copy link
Contributor

Is there any news on this? As per my previous post, RTK doesn't solve the problem, but the master version of redux-thunk does, so a release would be great.

@markerikson
Copy link
Contributor

markerikson commented Oct 23, 2021

Update

The primary concern here was that the use of global overloads of the bindActionCreators and Dispatch types from the Redux core module was the wrong approach. While most Redux apps do have the thunk middleware installed, permanently altering those types as a default behavior is not a good idea (same as globally mutating prototypes in JS code).

After some discussion, we've come up with an alternate approach: we've split the override types out into a standalone file that can be imported separately if desired.

That way, the default behavior reflects only what is true about a basic Redux store, and if you want to globally override the types in your app, you can do import 'redux-thunk/extend-redux'.

See #321 for the changes.

Beyond that, the other unpublished changes are:

  • Giving all the generic args meaningful names (S -> State, etc)
  • Adding an additional overload to ThunkDispatch that accepts a type union, which seems to fix some cases
  • Reordering the overloads in ThunkDispatch
  • Documentation of the types
  • A new ThunkActionDispatch type that appears to be intended to be meant for use with bindActionCreators

I would publish a new alpha right now, but I don't actually have NPM publish rights for the redux-thunk package yet. I will try publishing an alpha as soon as I have those rights added.

@markerikson
Copy link
Contributor

markerikson commented Oct 24, 2021

I'm still waiting to get publish rights for the NPM package, but I've just gotten CodeSandbox CI integrated into the repo and generated a test build over in #324 . If anyone wants to override their installed versions and try this out, you can use the CodeSandbox CI builds:

# yarn 1
yarn add redux-thunk@https://pkg.csb.dev/reduxjs/redux-thunk/commit/279ee262/redux-thunk
# yarn 2, 3
yarn add redux-thunk@https://pkg.csb.dev/reduxjs/redux-thunk/commit/279ee262/redux-thunk/_pkg.tgz
# npm
npm i redux-thunk@https://pkg.csb.dev/reduxjs/redux-thunk/commit/279ee262/redux-thunk

Would appreciate some feedback on whether this really does fix any outstanding TS issues for folks!

@timdorr
Copy link
Member

timdorr commented Oct 25, 2021

We're gearing up to do one soon, so I'm going to close this out.

@timdorr timdorr closed this as completed Oct 25, 2021
@markerikson
Copy link
Contributor

Yep. Tim gave me publish rights and did some cleanup. I will probably have 2.4 live tonight or tomorrow.

@markerikson
Copy link
Contributor

@Philipp91
Copy link
Contributor

Thank you! 2.4.0 works for me even without the suggested import 'redux-thunk/extend-redux'.

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

No branches or pull requests

10 participants