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

Add support of getStaticProps and getServerSideProps #196

Merged
merged 16 commits into from
May 11, 2020
Merged

Conversation

kirill-konshin
Copy link
Owner

Fix #192 #173
Mention #194

@dpyzo0o
Copy link

dpyzo0o commented Apr 1, 2020

When are you going to publish this?

@kirill-konshin
Copy link
Owner Author

@dpyzo0o it has been published as 6.0.0-rc.1, you can give it a shot

@dpyzo0o
Copy link

dpyzo0o commented Apr 1, 2020

@dpyzo0o it has been published as 6.0.0-rc.1, you can give it a shot

thx

@dpyzo0o
Copy link

dpyzo0o commented Apr 1, 2020

I have this error after upgrading to 6.0.0-rc.1

Screen Shot 2020-04-01 at 15 02 42

@kirill-konshin
Copy link
Owner Author

The API has changed - this is a major release. Please have a look in the upgrade section and demos. You will have to do some refactoring.

@jwkellyiii
Copy link

@kirill-konshin I am confused on something. In the README, under "Upgrade from 5.x to 6.x" you say "App should no longer wrap it's children with Provider". However, in this commit (1204602) you do exactly that in the packages/demo/src/pages/_app.tsx file. So should the README be fixed or is there something else going on?

Also, when I do have my App wrap its children with a Provider I get "TypeError: Cannot read property 'getState' of undefined", take out the Provider and the error goes away.

@kirill-konshin
Copy link
Owner Author

@jwkellyiii it just lost in numerous rebases and fixes. It's all good now as you can see in dev branch: https://github.com/kirill-konshin/next-redux-wrapper/blob/dev/packages/demo/src/pages/_app.tsx

@pcherm
Copy link

pcherm commented Apr 3, 2020

Hi @kirill-konshin. I'm a little bit confused. So in my project, I use custom _app, which wraps the page components with several layers of different providers. Can I still use getStaticProps in my pages? If so, how do I do that, as I can't get the props from my page's getStaticPrips passed down to the page via the _app pageProps.

@jwkellyiii
Copy link

@kirill-konshin so this is the right version: https://github.com/kirill-konshin/next-redux-wrapper/blob/dev/packages/demo/src/pages/_app.tsx

Meaning, don't wrap your App children in a Provider?

@kirill-konshin
Copy link
Owner Author

kirill-konshin commented Apr 3, 2020

@jwkellyiii you don't need to wrap with Provider.

@chermie getStaticProps and getServerSideProps aren't called in _app. You can use them at page level. If you're using _app then you basically cancel the advantages of per-page configuration.

@kirill-konshin
Copy link
Owner Author

@chermie moreover, I found this issue vercel/next.js#11648 — when you have _app and getStaticProps at page level, props returned from _app are silently ignored.

I fixed the order and released 6.0.0-rc.2: https://github.com/kirill-konshin/next-redux-wrapper/releases/tag/6.0.0-rc.2

@zlwaterfield
Copy link

Really looking forward to testing this, it will have huge improvements on performance with the SSG support. Thanks for the quick turn around on this!

@kirill-konshin
Copy link
Owner Author

@zlwaterfield please let me know if it works as expected. I don't have enough time now to test all possible cases, so if you can give it a shot and provide some feedback, that would be great.

@jwkellyiii
Copy link

@kirill-konshin I just want to make a note that using redux-saga is not as easy as mentioned here (https://github.com/kirill-konshin/next-redux-wrapper#usage-with-redux-saga) if you use getServerSideProps.

@jwkellyiii
Copy link

@kirill-konshin are you going to push rc.2 to npm?

Deps upgrade and fix for props order
@kirill-konshin
Copy link
Owner Author

@kirill-konshin
Copy link
Owner Author

@kirill-konshin I just want to make a note that using redux-saga is not as easy as mentioned here (https://github.com/kirill-konshin/next-redux-wrapper#usage-with-redux-saga) if you use getServerSideProps.

Can you tell me more, I'm not familiar with sagas, what is the problem?

@jwkellyiii
Copy link

jwkellyiii commented Apr 7, 2020

@kirill-konshin it is all async requests and getServerSideProps that are the issue. I'll give more details when I can, but it is more then sagas that are the problem (same thing with redux-thunk, etc.).

@kirill-konshin
Copy link
Owner Author

@jwkellyiii same as before you just need to await for all asynchronous actions to finish. Nothing changed in this aspect.

@pcherm
Copy link

pcherm commented Apr 7, 2020

@jwkellyiii I couldn't get sagas to work either. It doesn't seem to wait for the async actions to resolve and as the result no data lands in the store, it was working fine before I upgraded to rc1

@kirill-konshin
Copy link
Owner Author

- Redux Saga example
- Proper support of `getStaticProps` and `getServerSideProps`
- Removed `isServer`
- Better tests
@na-ji
Copy link

na-ji commented Apr 22, 2020

Thanks @kirill-konshin it works now, on the demo and on my own app. It was the only problem I found, so thank you very much! 👍

@phuson
Copy link

phuson commented Apr 22, 2020

@kirill-konshin Hmm, after updating to 6.0.0-rc.8, I'm getting this warning in the console log after navigating between pages that use the wrapper (i.e. wrapper.withRedux(About))

 Warning: Cannot update a component (`Unknown`) while rendering a different component (`withRedux(About)`). To locate the bad setState() call inside `withRedux(About)`, follow the stack trace as described in https://fb.me/setstate-in-render
    in withRedux(About) (at _app.tsx:13)
    in MyApp
    in Container (created by AppContainer)
    in AppContainer index.js:1
    e index.js:1
    r react_devtools_backend.js:6
    NextJS 5
        printWarning
        error
        warnAboutRenderPhaseUpdatesInDEV
        scheduleUpdateOnFiber
        dispatchAction
    Redux 3
        checkForUpdates
        handleChangeWrapper
        notify
    batchedUpdates$1 NextJS
    Redux 4
        notify
        notifyNestedSubs
        handleChangeWrapper
        dispatch
    e about:1
    sagaMiddleware Redux
    dispatch about:1
    Redux 2
        hydrate
        Wrapper
    NextJS 16
        renderWithHooks
        mountIndeterminateComponent
        beginWork
        beginWork$1
        performUnitOfWork
        workLoopSync
        performSyncWorkOnRoot
        flushSyncCallbackQueueImpl
        unstable_runWithPriority
        runWithPriority$1
        flushSyncCallbackQueueImpl
        flushSyncCallbackQueue
        scheduleUpdateOnFiber
        updateContainer
        legacyRenderSubtreeIntoContainer
        render
    renderReactElement index.js:38
    doRender$ index.js:44
    tryCatch runtime.js:45
    invoke runtime.js:274
    method runtime.js:97
    tryCatch runtime.js:45
    invoke runtime.js:135
    callInvokeWithMethodAndArg runtime.js:170
    callInvokeWithMethodAndArg runtime.js:169
    enqueue runtime.js:192
    method runtime.js:97
    async runtime.js:219
    doRender index.js:40
    render$ index.js:25
    tryCatch runtime.js:45
    invoke runtime.js:274
    method runtime.js:97
    tryCatch runtime.js:45
    invoke runtime.js:135
    callInvokeWithMethodAndArg runtime.js:170
    callInvokeWithMethodAndArg runtime.js:169
    enqueue runtime.js:192
    method runtime.js:97
    async runtime.js:219
    render index.js:25
    subscription index.js:23
    notify router.js:569
    set router.js:436
    change router.js:311

It only appears once per new page that I'm navigating to. The warning does not appear if I navigate away and back to that particular page. FYI, all of the pages have their own getServerSideProps(). Do you happen to know what this new warning is all about?

@kirill-konshin
Copy link
Owner Author

@phuson I've seen this warning and it was supposed to go away because of this special treatment: https://github.com/kirill-konshin/next-redux-wrapper/blob/dev/packages/wrapper/src/index.tsx#L180-L190

Can you check how your pages are mounted? Is there any special treatment? If you can rearrange the demos to show this behavior I'd appreciate it.

@zlwaterfield
Copy link

zlwaterfield commented Apr 24, 2020

@kirill-konshin I am running into two issues and I think they are related.

First: I am seeing the hydration with an empty payload:

{ type: '__NEXT_REDUX_WRAPPER_HYDRATE__', payload: undefined }

It seems to only happen on pages where I do not have getInitialProps, the pages with getInitialProps work as expected. Is it possible to make it an empty object (or in the case of serialization an empty immutable Map)?

Right now the hydration fails unless I add a check like this:

    case HYDRATE:
      return state.merge(
        action.payload ? action.payload.get('Account') : {}
      );

Second: I wrapped my page in an HOC and I am trying to hoist getServerSideProps with this but it is not being called:

  if (WrappedComponent.getServerSideProps) {
    CorePageWrapper.getServerSideProps = wrapper.getServerSideProps(WrappedComponent.getServerSideProps);
  }

But hoisting getInitialProps with this works

  if (WrappedComponent.getInitialProps) {
    CorePageWrapper.getInitialProps = WrappedComponent.getInitialProps;
  }

Any ideas here? I realize I must be doing something wrong with how I am hoisting it.

@kirill-konshin
Copy link
Owner Author

@zlwaterfield you need to hoist so-called "non-react statics" yourself if you use any HOCs on top of Wrapper. Here's the package: https://github.com/mridgway/hoist-non-react-statics.

@phuson any chance to get a reproducible case for your warning issue? It works for me so far...

@zlwaterfield
Copy link

zlwaterfield commented May 3, 2020

@kirill-konshin I got it working, thanks for the link.

One question, why does payload return undefined if you wrap a page with wrapper.withRedux(...) and don't call one of the static methods (getInitialProps, getServerSideProps, getStaticPaths, getStaticProps). The HYDRATE is called but the payload is undefined.

Currently, my workaround is, as mentioned above, to only HYDRATE the payload if it exists:

    case HYDRATE:
      return state.merge(
        action.payload ? action.payload.get('Account') : initialState
      );

The reason I need this is that my HOC that wraps every page is using the wrapper, most pages call one of the static next methods but a few pages don't. This HOC has auth checks and some layout related items, so it would be a pain to split it into two HOCs where some pages use both and some pages use only one.

@kirill-konshin
Copy link
Owner Author

@zlwaterfield:

why does payload return undefined if you wrap a page with wrapper.withRedux(...) and don't call one of the static methods

            const hydrate = useCallback(() => {
                store.current.dispatch({
                    type: HYDRATE,
                    payload: getDeserializedState(initialState, config),
                } as any);

                // ATTENTION! This code assumes that Page's getServerSideProps is executed after App.getInitialProps
                // so we dispatch in this order
                if (initialStateFromGSPorGSSR)
                    store.current.dispatch({
                        type: HYDRATE,
                        payload: getDeserializedState(initialStateFromGSPorGSSR, config),
                    } as any);
            }, [initialStateFromGSPorGSSR, initialState]);

As you see it just uses initial state, which is empty, because wrapper provides it, so no wrapper no state.

@zlwaterfield
Copy link

Ok thanks, I can stick with my workaround for now.

@kirill-konshin
Copy link
Owner Author

So looks like everything is fairly stable now, I'm not seeing any major issues. If this is right, I am going to promote this stuff to a major release.

One thing that's left is @phuson's case, but like I said, code has been reworked to work around this particular issue. Can you please provide a reproducible case or confirm that it is now fixed?

@zlwaterfield
Copy link

I've had this running with >50 routes on our staging env for a few days now and everything is looking good to us!

@phuson
Copy link

phuson commented May 8, 2020

@kirill-konshin Sorry for the late response. I got caught up in another project and couldn't return to this until now.

Last time I was testing it, as I recollect, it seems like the conversion from the the class based component implementation (r.6) to the new functional component with hooks (r.8) caused the warning to be thrown in the console log. I spent some times debugging but couldn't figure out exactly why the warning was thrown. Before I was pulled away from this project, I had postulated that it could be that some functionalities were acting differently with the conversion (hooks vs lifecycle functions).

Fast forward to today, I'm trying to get back into the project and I am no longer seeing the warning. So for now, please disregard my issue above. I will continue to test it further and will let you know if the issue comes up again. Thanks again for working on this!

@kirill-konshin
Copy link
Owner Author

@phuson old version indeed might had this issue. Thank you for confirming.

@kirill-konshin kirill-konshin merged commit 76d9eb1 into master May 11, 2020
@kirill-konshin kirill-konshin deleted the dev branch May 11, 2020 21:12
@kirill-konshin
Copy link
Owner Author

Thanks to everyone in this thread, you guys helped tremendously to shape and test the new feature. It is now released as next major version: 6.0.0.

@tkstang
Copy link

tkstang commented Jun 8, 2020

Second: I wrapped my page in an HOC and I am trying to hoist getServerSideProps with this but it is not being called:

  if (WrappedComponent.getServerSideProps) {
    CorePageWrapper.getServerSideProps = wrapper.getServerSideProps(WrappedComponent.getServerSideProps);
  }

But hoisting getInitialProps with this works

  if (WrappedComponent.getInitialProps) {
    CorePageWrapper.getInitialProps = WrappedComponent.getInitialProps;
  }

Any ideas here? I realize I must be doing something wrong with how I am hoisting it.

@zlwaterfield Would you mind sharing an example of how you were able to successfully implement an HOC with getServerSideProps? The library that was linked appears to hoist static functions of the wrapped component to the wrapper. What confuses me about how you got this to work is that the Next docs explicitly state Also, you must use export async function getServerSideProps() {} — it will not work if you add getServerSideProps as a property of the page component.

@kirill-konshin
Copy link
Owner Author

kirill-konshin commented Jun 8, 2020

You need to export const getServerSideProps not like static WrappedCmp.getInitialProps

@tkstang
Copy link

tkstang commented Jun 8, 2020

You need to export const getServerSideProps not like static WrappedCmp.getInitialProps

I understand that, but since getServerSideProps is an exported function of a component and not a static function how is it possible to hoist getServerSideProps? I only ask because his question was about how he could hoist getServerSideProps and then he said he got it working.

@kirill-konshin
Copy link
Owner Author

What do you mean "to hoist"? It is not hoisted, Nextjs automatically imports this function from the page.

@zlwaterfield
Copy link

zlwaterfield commented Jun 8, 2020

@tkstang, as @kirill-konshin mentioned you can export the function directly from the page since it is not static, you don't need to hoist it.

Here is a slimmed-down page where withPageWrapper is my HOC wrapped around each page:

import { wrapper } from '@redux/store';

import { withPageWrapper } from '@components/CorePageWrapper';

export const getServerSideProps = wrapper.getServerSideProps(
  ({ res }) => {
    ...
  }
);

class Page extends Component {

  ... 
  
  render() {
    return (
      <>
        ...
      </>
    );
  }
}

export default withPageWrapper(Page);

@tkstang
Copy link

tkstang commented Jun 9, 2020

I see. I misunderstood and thought you had determined a way to export a getServerSideProps function from your core page wrapper HOC that allowed you to have some serverSideProps logic shared between all of your pages.

@zlwaterfield
Copy link

Ya sorry, my explanation wasn't great, I was still trying to understand how it all works.

Also, I wouldn't recommend having serverSideProps logic shared between all of your pages unless you really need it because if you have getServerSideProps or getInitialProps then the page won't be statically optimized which means it will be rendered each time its called instead of being built at build time and serving the pre-built HTML file. The new solution @kirill-konshin released in v6 for a per-page wrapper allows you to statically optimize pages that don't need server-side data.

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.

new next.js 9.3.0 getServerSideProps is not getting modified context