kirill-konshin / next-redux-wrapper

Redux wrapper for Next.js
MIT License
2.67k stars 265 forks source link

2.0.0 breaking changes? #89

Closed hrdymchl closed 6 years ago

hrdymchl commented 6 years ago

@kirill-konshin this is a great library but we're having issues upgrading to 2.0.0-beta.4 from 1.3.5. We are using Next 6.0.0. Can you kindly add documentation on the breaking changes?

DonovanCharpin commented 6 years ago

Hi @hrdymchl, I just did the migration from next 4.X -> next 6.0.0 and the next-redux-wrapper migration from 1.3.5 to 2.0.0-beta.4.

I think the main thing I had to do was to migrate my main Wrapper (hoc) to the _app.js, after that my code was cleaner and I started the migration.

I just checked my commit and this is the only thing I changed. Check the readme, it gives also some code to reuse when doing the _app.js migration to integrate with the next-redux-wrapper.

I have still one thing I don't know how to fix. The props from mapStateToProps are not sent to MyApp so maybe there is a breaking change with this. I don't find in the doc where I can pass this :

//_app.js

const mapStateToProps = state => {
  return {
    translations: state.translations.entities,
    config: state.config.entities,
    theme: state.theme.entities
  };
};

export default withRedux(initStore, mapStateToProps)(MyApp);
kirill-konshin commented 6 years ago

MyApp does not receive by design in order to keep the interface as minimal as possible :) you can return whatever you want from MyApp's getInitialProps, or if you need a shared layout just create it and connect it as usual, then include it either in the page itself or render in MyApp like so:

import React from 'react'
import withRedux from "next-redux-wrapper";
import {makeStore} from "../components/store";
import ConnectedLayout from "../components/Layout";

export default withRedux(makeStore, {debug: true})(class MyApp extends React.Component {

    static async getInitialProps({Component, ctx}) {

        return {
            pageProps: {
                // Call page-level getInitialProps
                ...(Component.getInitialProps ? await Component.getInitialProps(ctx) : {})
            }
        };

    }

    render() {
        const {Component, pageProps} = this.props;
        return (
            <ConnectedLayout>
                <Component {...pageProps} />
            </ConnectedLayout>
        );
    }

});

It is possible because MyApp is already wrapped with <Provider>. Separation of concerns rocks :)

I have updated the readme with upgrade instructions.

DonovanCharpin commented 6 years ago

Hi @kirill-konshin,

I see but that could be nice to be able to push mapStateToProps and mapDispatchToProps in the withRedux method. The use case is simple here with MyApp but on my side, I have multiple providers for theme injection (styled component), translation injection and toggle-feature to inject permissions. I get this config in getInitialProps and it's stored in multiple reducers. Then I want to inject them before rendering through mapStateToProps. I think it should be an option for more advanced use cases.

Create another Wrapper just for this doesn't really make sense and I will have to give the req to this layer, just to be able to do the job.

kirill-konshin commented 6 years ago

But that's how React HOC composition should work, preserving the single responsibility principle, you can wrap multiple times and each HOC will bring it's own functionality:

withRedux(makeStore)(connect(...)(class MyApp extends React.Component {...}));
DonovanCharpin commented 6 years ago

Sure I agree with that. It's just that for me, this hoc should be responsible for mapStateToProps and mapDispatchToProps because it's in the default connect of react-redux. I'll do a second wrap if it's not possible to implement it with the library.

Anyway this library is really hot, good job 👍

kirill-konshin commented 6 years ago

Thanks for your interest in the lib!

simplynutty commented 6 years ago

@kirill-konshin Do you have an example of how to pass through mapStateToProps and mapDispatchToProps? My next js page looks like this and was working fine before upgrading to 2.0.

const withRedux = connect(mapStateToProps, mapDispatchToProps);

export default compose(
  withGraphQL,
  withRedux,
  withCheckoutQuery,
  withStoredCardsQuery,
  withInsuranceQuery,
)(Checkout);

I'm using ApolloClient so I don't think I need to include <Provider/> in _app.js.

kirill-konshin commented 6 years ago

Just follow the instructions here: https://github.com/kirill-konshin/next-redux-wrapper#upgrade

Apollo does not come with Redux, so you still need Provider.

simplynutty commented 6 years ago

I already followed the upgrade instructions. I'm close but have 1 error left.

Could not find "store" in either the context or props of "Connect(Apollo(Apollo(Apollo(CheckoutContainer))))". Either wrap the root component in a <Provider>, or explicitly pass "store" as a prop to "Connect(Apollo(Apollo(Apollo(CheckoutContainer))))".

_app.js is using the same code shown in the upgrade instructions.

kirill-konshin commented 6 years ago

Looks like you don't have Provider in your chain of components. I can't diagnose w/o seeing the code.

simplynutty commented 6 years ago

Sorry. https://gist.github.com/simplynutty/c2daeb74acef6fdfe672c80d65d85c74 I'm using Provider in _app.js just like the example. I don't need redux on the server. This pattern was working fine with Next JS 5 and next-redux-wrapper 1.x so I'm hoping I can figure this out. Also recently went to babel 7.

kirill-konshin commented 6 years ago

I bet the problem is here: https://gist.github.com/simplynutty/c2daeb74acef6fdfe672c80d65d85c74#file-withgraphql-js-L40 bc this place is not wrapped with Provider.