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

Fix assignment of props in WithApollo.getInitialProps #11236

Merged
merged 1 commit into from
Mar 24, 2020

Conversation

bgoerdt
Copy link
Contributor

@bgoerdt bgoerdt commented Mar 20, 2020

The props are being assigned incorrectly based on the inAppContext, they should be wrapped in pageProps when inAppContext is true, not the other way around. This bug will cause a new apollo client to be created during getDataFromTree instead of using the one that has already been created on the server.

The props are being assigned incorrectly based on the `inAppContext`, they should be wrapped in `pageProps` when `inAppContext` is true, not the other way around. This will cause a new apollo client to be created during `getDataFromTree` instead of using the one that has already been created on the server.
@bgoerdt bgoerdt requested review from lfades and Timer as code owners March 20, 2020 18:50
Copy link
Member

@lfades lfades left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bgoerdt Thank you!

@timneutkens timneutkens changed the base branch from master to canary March 24, 2020 09:11
@timneutkens timneutkens merged commit db57ad3 into vercel:canary Mar 24, 2020
@antoniojps
Copy link

antoniojps commented Mar 25, 2020

Hey, this is unrelated to this PR. However I couldn't find a better place to ask.

In the following snippet...

 // We consider installing `withApollo({ ssr: true })` on global App level
  // as antipattern since it disables project wide Automatic Static Optimization.
  if (process.env.NODE_ENV === 'development') {
    if (inAppContext) {
      console.warn(
        'Warning: You have opted-out of Automatic Static Optimization due to `withApollo` in `pages/_app`.\n' +
          'Read more: https://err.sh/next.js/opt-out-auto-static-optimization\n'
      )
    }
  }

...the comment says that it is an antipattern to use withApollo({ ssr: true }) on global App level. So is it an antipattern to use with SSR set to false on the App level?

If not, shouldn't this warning be ignored if SSR is false?

antoniojps pushed a commit to antoniojps/voluntarios that referenced this pull request Mar 25, 2020
@lfades
Copy link
Member

lfades commented Mar 25, 2020

@antoniojps It should be ignored, yes. If that's not the case you're welcome to open a PR to fix it.

@zanettin
Copy link

hi @bgoerdt 👋
since this PR both variations (hoc on _app and hoc on pages) do no longer work on my end. data are just fetched on the client and not on ssr. you can also try the example locally and check NEXT_DATA and search for the gql request on the network tab. note that the api has reached it's limit so i've tested the example code agains swapi.graph.cool and rewrote the post component to fetch films out of it.

is it possible that you re-check it and maybe undo your changes? for me it's not an issue since i have a custom solution anyway but maybe for others it could be misleading. thanks for your contribution 👍

@bgoerdt
Copy link
Contributor Author

bgoerdt commented Mar 26, 2020

@zanettin yep I'll revert it. I'm not sure why it's working the opposite way in my project, but the example project is broken with my change. Sorry about that.

bgoerdt added a commit to bgoerdt/next.js that referenced this pull request Mar 26, 2020
@zanettin
Copy link

@bgoerdt top. thx a lot 👍 nave a nice weekend

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants