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

Deprecating connectAdvanced #1236

Closed
timdorr opened this issue Apr 12, 2019 · 25 comments · Fixed by #1771
Closed

Deprecating connectAdvanced #1236

timdorr opened this issue Apr 12, 2019 · 25 comments · Fixed by #1771

Comments

@timdorr
Copy link
Member

timdorr commented Apr 12, 2019

I intend to remove the connectAdvanced API from React Redux, essentially replacing connect with its contents.

Why? It's an abstraction that we don't use ourselves, so it just becomes an indirection in the code. It is only minimally used in the community. It creates a more complex code base, which is a barrier to entry for many contributions. Plus, frankly, I just don't like it; it gives me Java flashbacks.

This is mainly to give the few folks that do use it a heads up. It won't go away until the next major release. However, I intend to add a deprecation warning at some point in the future. We'll do that after Hooks, just so we're not dropping two big changes in one minor release. Plus, you can lock to that version to have your Hooks and your connectAdvanced at the same time. Sound fair?

If there are specific use cases, we can see about migrating them into the connect API, most likely as options. This isn't about completely throwing away a big chunk of functionality, just simplifying the code base a bit and making it easier to grok for everyone.

@timdorr timdorr pinned this issue Apr 12, 2019
@gaearon
Copy link
Contributor

gaearon commented Apr 12, 2019

👍 I could never understand what it does or why it was added. It sounds like if you need this kind of complexity you should just vendor the connect code and maintain it yourself.

@markerikson
Copy link
Contributor

markerikson commented Apr 12, 2019

FWIW, the intended use case is for an end user to be able to customize how all the memoization and prop generation is handled, on their own. All the logic for managing the subscription itself stays the same - it's really just changing from calling mapState and mapDispatch specifically, to (storeState, dispatch) => childProps, and connect implements its mapState/mapDispatch handling as one possible (storeState, dispatch) => childProps function.

If y'all are really dead set on removing it, we can, but I truly don't think it adds that much extra complexity.

Lemme tag three folks who I know have a particular interest in this: @jimbolla, @epeli, @mariusandra.

A couple of example usages:

https://github.com/keajs/kea/blob/9491b5a269b4c18644dc4185d43f77da3d7a79d1/src/kea/index.js#L257

https://github.com/epeli/redux-render-prop/blob/7e1fcf4d38b41616f56f3c53eb15233610570215/src/redux-render-prop.ts#L72

@esamattis
Copy link
Contributor

esamattis commented Apr 12, 2019

Bummed about this of course but I understand the reasoning and I can live with. Actually just realized I can achieve everything I want with the areStatePropsEqual option of connect().

Farewell connectAdvanced().

@mariusandra
Copy link

This would be a bummer. Kea is basically built around it. I'll need to see if I can work around it and will provide a more insightful reply next week, as I'm literally boarding a plane for a weekend getaway now.

Cheers!

@jimbolla
Copy link
Contributor

I'm not using Redux so I don't have an opinion on this.

@mariusandra
Copy link

Hey, after tinkering a bit with the code (read: totally rewriting Kea), I no longer need connectAdvanced.

Now that I worked with the code again, I remember why I used it in the first place: I was being sneaky, grabbed the dispatch that was given to selectorFactory and used it to initialise the redux store under certain conditions. I remember I couldn't find any other clean way to directly get dispatch from the react-redux API without caching it on store creation, so I went with connectAdvanced. It simplified a few other things as well... and made many others way too complicated.

Now I bit the bullet and switched to just stealing the store from react-redux and using it directly to dispatch hydration actions in order to regenerate the store when a new reducer needs to be added. I'm not sure if dispatching actions to regenerate the store is better than using store.replaceReducer or not... but it has never caused issues so far.

The rewrite comes with many other changes, some of which are breaking, so it's not released yet. However I got all the tests to pass, so it shouldn't be too long now. I still need to rearchitect the plugin system, integrate thunks and sagas and test it on my own app before I'm ready to release it into the wild.

Thanks for the push guys! This rewrite was long overdue!

@markerikson
Copy link
Contributor

Well, based on those responses, it sounds like the primary folks who were using it have determined that they no longer need it. I'd say that puts a nail in it.

(I still don't think there's going to be much actual difference as far as the internal code goes, given the way things are structured, but we'll see.)

@flipmotion
Copy link

flipmotion commented Apr 24, 2019

Hi to all!
I think connectAdvanced is usefull, basically connect are sliced to 3 part for work with redux and i think it's like tricky. I used connectAdvanced around in our project BUSFOR, and thats awesome:

`

export default connectAdvanced(dispatch => {
  const getActions = {some code};

  let result;

  return ({ search }, ownProps) => {
    const stateProps = ({
      {...}
      ...getActions({ tripId, selectedSeats, bestPrice }),
    });

    const nextProps = {
        ...ownProps,
        ...stateProps,
        dispatch,
     };

    if (!equals(result, nextProps)) {
      result = nextProps;
    }

    return result;
 }; 
})(Trip);

`

For my think, it's cleaner way to put some data from store\from view into Actions.

Or mb i do something wrong?

PS: and no need it more selectors func from reselect, so i do my self with equals of next props

@markerikson
Copy link
Contributor

@flipmotion : would one of the other options for customizing connect's comparisons work for your situation?

@flipmotion
Copy link

flipmotion commented Apr 24, 2019

@markerikson : I think it could be, but it’s, again, more useless code. In ‘connectAdvanced’ it better cuz ur get all props from state, from component it one place, and if u need just 2-3 things, it’s more simple. Again i create selectors for data in first call, then i can combine it in second calling with some storeProps and OwnProps. And the same with actionCreators like with computed data selection. And this do in one place, without cutting it for other functions in connect options.

const mapDispatchToProps = (dispatch) => {
  return {
     dispatchSave: (items) => dispatch({type: "SAVE_ALL", items})
  };
};

const mergeProps = (propsFromState, propsFromDispatch, ownProps) {
  return {
    save: propsFromDispatch.dispatchSave(propsFromState.items)
  };
};

const SaveAllContainer = connect(mapStateToProps, mapDispatchToProps, mergeProps)(SaveAll)

I think it more boilerplate then connectAdvanced.

export default connectAdvanced(dispatch => {
  const getActions = {some code with createSelector};

  let result;

  return ({ storeProp }, { ownProp, ...ownProps }) => {
    const stateProps = {
      ...getActions({ storeProp, ownProp }),
    };

    const nextProps = {
        ...ownProps,
        ...stateProps,
        dispatch,
     };

    if (!equals(result, nextProps)) {
      result = nextProps;
    }

    return result;
 }; 
})(Component);

Like i in my previous post when i can put some data from store and ownProps in actionCreator in one simple way.

I understand that this is a trick, but I am aware of my actions. I consider it a very simple and effective solution.

Or i do something wrong? Thx.

@goszczynskip
Copy link

goszczynskip commented Jun 18, 2019

Connecting Deck.gl layers is very convenient with connectAdvanced. Layer classes can't be wrapped directly with connect because they are not React components. To create them you can leverage connectAdvanced's feature of getting dispatch and state in one function.

const selector = dispatch => {
  const actions = mapDispatchToProps(dispatch);
  const getLayers = createLayersSelector(
    dispatch
  );

  return (nextState, nextOwnProps) => {
    const nextProps = mapStateToProps(nextState, nextOwnProps);
    const layers = getLayers(nextState, nextOwnProps);
    return {
      ...actions,
      ...nextProps,
      layers
    };
  };
};

layers selector:

export default dispatch => {
  const getData = (state) => state // here you can prepare selector

  return createSelector(
    [getData],
    (data) => {
      return [new PointsLayer({
        data,
        onClick: bindActionCreators({ someAction }, dispatch) // here you need dispatch 
        // other layer props
      })];
    }
  );
};

Such approach allows you to split layer creation to different files and keep each layer near to it's own selectors. I can't see how presented solution (or similar) may be achieved with mergeProps from connect.

@kogal
Copy link

kogal commented Nov 22, 2019

I would suggest that you add a deprecation note in the connectAdvanced's documentation, to prevent people, new to react-redux, from using it.

https://react-redux.js.org/api/connect-advanced

@whykushal93
Copy link

If we are deprecating it, could we add ownProps as an argument to the areStatesEqual option please ? I was hoping to use connectAdvanced to get around ownProps not being passed to areStatesEqual.
This was previously asked in this and there was also a PR.

@sktguha
Copy link

sktguha commented Jan 12, 2021

Hello what is the final decision being made ? Is connectAdvanced going to be deprecated ?
Based on this thread it seems to be the case, but just needed confirmation that it is going to be deprecated.

@markerikson
Copy link
Contributor

Oh, it's very deprecated now that the hooks API exists.

@sktguha
Copy link

sktguha commented Jan 12, 2021

Ok understood. Thanks. But will it actually be deleted from the code and connect used instead as timdorr was mentioning in his initial post ?
I believe no, as for class components no further work is going to be done except bug fixes and minor fixes.
Its still gonna remain then as it is right now ?

@markerikson
Copy link
Contributor

No changes for the lifetime of the v7.x branch.

When we finally get around to doing a version 8, we will definitely still have connect - I just expect the internal implementation to be consolidated so there's no separate connectAdvanced.

No concrete plans for a version 8, yet, but I expect that we'll tackle it once Suspense and Concurrent Mode are finally done and the useMutableSource hook is released to production.

@sktguha
Copy link

sktguha commented Jan 12, 2021

Alright Mark, thanks for providing a detailed update.

once Suspense and Concurrent Mode are finally done

This seems interesting. Any blog post and/or WIP branch where I can know more about this effort ?

@markerikson
Copy link
Contributor

Just the React repo, really.

I'm sure once CM/Suspense are truly "done" there will be a post on the React blog announcing that.

@sktguha
Copy link

sktguha commented Jan 12, 2021

oops my bad. Got confused. I thought React-Redux was doing some specific work for Suspense and concurrent mode support haha.

@sktguha
Copy link

sktguha commented Jan 12, 2021

the useMutableSource hook is released to production

And what about class components 😕 ? I didn't see any plans anywhere to expose such an API for class components.

@markerikson
Copy link
Contributor

I'm not sure what you're asking here.

useMutableSource is an as-yet-unreleased React API that will allow external state management tools to better integrate with Concurrent Mode.

The React team is definitely not adding any new features to support class components.

As for React-Redux, connect itself was reimplemented as a function component in v7, and uses hooks inside.

I'm not entirely sure if we'll be able to fully rework connect to use useMutableSource inside or not, or whether it'll only be useSelector that can benefit. Won't know until all this stuff is actually finalized and available for us to use.

As I said, questions like this are best suited for the React repo, not here.

@sktguha
Copy link

sktguha commented Jan 12, 2021

yes yes sure. sorry about that. will try to find some other forum for that. I was asking because my app is using class components and connect API, as we are using Redux for state management . So was wondering if we would be able to use concurrent mode properly without big refactorings like changing components to hooks etc. I will wait for the concurrent mode to be "done" from the React team in that case to get a clearer answer.

@markerikson
Copy link
Contributor

Update: I've just put up #1771 , which does actually remove the connectAdvanced export and implementation entirely. There's now just a single function, named connect.

The PR is a draft atm, but I intend to merge it after the TypeScript port in #1739 gets merged into master.

@markerikson
Copy link
Contributor

And this API is now gone as of v8.0.0-alpha.0!

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 a pull request may close this issue.