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

getInitialProps is called twice #77

Closed
haoict opened this issue Jan 4, 2019 · 22 comments
Closed

getInitialProps is called twice #77

haoict opened this issue Jan 4, 2019 · 22 comments

Comments

@haoict
Copy link

haoict commented Jan 4, 2019

Hi.
As title above, I created codesandbox. Please take a look.
https://codesandbox.io/s/l2838zpq1q

@duongkimngoc
Copy link

same issue here 😢

@devrasec
Copy link

devrasec commented Jan 4, 2019

sadly I have the same issue

@isaachinman
Copy link
Contributor

So, it seems like this is not related to any Redux HOC, although it appears using the Redux HOC is causing the getInitialProps on the _app.js component to get swallowed or otherwise not run.

I'm still seeing two Index::getInitialProps logs even after removing the withRedux HOC.

When submitting a bug report, it's very helpful to try and narrow it down to the simplest possible reproducible example.

I'll look into this further now. Thanks for your patience everyone.

@isaachinman
Copy link
Contributor

So - getInitialProps is being called twice because you're calling it yourself. You don't need to execute the if (Component.getInitialProps) clause - we're already doing that internally within next-i18next.

I believe this was just a case of misunderstanding and copy/pasting from docs. Let me know if this is clear, and if anyone thinks something should be changed.

@haoict
Copy link
Author

haoict commented Jan 5, 2019

@isaachinman
I think the problem is not related to withRedux anymore, but only appWithTranslation.
I removed all redux related things, and still seeing two Index::getInitialProps logs.
Please confirm and reopen this issue

https://codesandbox.io/s/l2838zpq1q

@haoict haoict changed the title getInitialProps is called twice when using next-redux-wrapper HOC getInitialProps is called twice Jan 5, 2019
@isaachinman isaachinman reopened this Jan 5, 2019
@isaachinman
Copy link
Contributor

isaachinman commented Jan 5, 2019

So: both Component and WrappedComponent have their getInitialProps function called within our appWithTranslation HOC, and weirdly it seems that both are being called onto the index.js level instead of once on _app.js and once on index.js.

If I explicitly do declare a getInitialProps on the _app.js level that doesn't contain another getInitialProps call, the problem does go away. Check out this sandbox.

My guess is that this has to do with hoisting statics and getInitialProps. I suppose we'll need some way of doing an equality check against WrappedComponent and Component.

@isaachinman
Copy link
Contributor

I created a branch called do-not-hoist-getInitialProps which I think fixes this problem.

However, I'm worried that it's also a regression and will break #11. Can a Redux user please test that branch with your compose / connect HOC usage and let me know if everything works?

@haoict
Copy link
Author

haoict commented Jan 7, 2019

I've tried to test do-not-hoist-getInitialProps branch, something is broken (in bootstrap.min.js ???). I leave my screenshot here because I'm not sure about things inside.

screen shot 2019-01-07 at 11 19 33

(In master branch, above error doesn't happen)

@isaachinman
Copy link
Contributor

@haoict I really doubt that's related to our codebase here, but I'd need a reproducible repo to take a look and debug further.

@isaachinman
Copy link
Contributor

Anyone get a chance to do some testing on that branch?

@hwangar
Copy link

hwangar commented Jan 14, 2019

I came across this issue in my project and confirmed your branch fixes it

@isaachinman
Copy link
Contributor

I know it fixes it. I'm worried about regression with #11.

@isaachinman
Copy link
Contributor

@capellini This is another issue that needs to be resolved before a prod release. Do you have any Redux/HOC projects to test with?

@capellini
Copy link
Contributor

capellini commented Jan 17, 2019

Do you have any Redux/HOC projects to test with?

@isaachinman, unforunately I don't. But in appWithTranslation, we shouldn't have to call both Component.getInitialProps and WrappedComponent.getInitialProps. Calling just WrappedComponent.getInitialProps should suffice. When you use this branch, do things work as expected?

See https://codesandbox.io/s/74l24m5pnj - seems to work in this use case.

@isaachinman
Copy link
Contributor

@capellini What's this loadGetInitialProps util you're pulling in? I don't think it's safe to use NextJs internals. Can you refactor your approach to both:

  1. Not use NextJs internals
  2. Retain hoisting of getInitialsProps?

If so, that'd be an ideal solution.

@capellini
Copy link
Contributor

@isaachinman loadGetInitialProps removed. See 2773495. Would this work, or do you foresee problems with this approach? I'll submit a PR so we can continue discussion there.

@haoict
Copy link
Author

haoict commented Jan 18, 2019

@capellini I think your PR will not resolve this issue.
I've tested it and found that SSR is not working properly. Could you double check?

@capellini
Copy link
Contributor

Yes, you're right. SSR isn't working...looking more into this.

@capellini
Copy link
Contributor

@haoict I think I've fixed the SSR issue. Could you try again and let me know what you find?

@haoict
Copy link
Author

haoict commented Jan 19, 2019

@haoict I think I've fixed the SSR issue. Could you try again and let me know what you find?

@capellini seems good now :D

@isaachinman
Copy link
Contributor

@capellini What was the SSR issue? Are you sure that this is safe to do?

@capellini
Copy link
Contributor

The issue was that, since this line was changed from:

    static async getInitialProps({ Component, ctx, ...initObject }) {

to:

    static async getInitialProps(ctx) {

so ctx.ctx === ctx in the old code. I had not accounted for this, so req was always undefined.

It is no less safe than what we were doing before.

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

6 participants