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

SSG pages re-render if middleware is being used #38267

Closed
1 task done
imangm opened this issue Jul 3, 2022 · 22 comments · Fixed by #38422
Closed
1 task done

SSG pages re-render if middleware is being used #38267

imangm opened this issue Jul 3, 2022 · 22 comments · Fixed by #38422
Labels
bug Issue was opened via the bug report template.

Comments

@imangm
Copy link

imangm commented Jul 3, 2022

Verify canary release

  • I verified that the issue exists in the latest Next.js canary release

Provide environment information

Operating System:
Platform: win32
Arch: x64
Version: Windows 10 Home
Binaries:
Node: 14.18.0
npm: N/A
Yarn: N/A
pnpm: N/A
Relevant packages:
next: 12.2.1-canary.2
eslint-config-next: 12.2.0
react: 18.2.0
react-dom: 18.2.0

What browser are you using? (if relevant)

Chrome 103.0.5060.66

How are you deploying your application? (if relevant)

next start, Vercel

Describe the Bug

By using the new stable middleware.js at the root of the project, all pages will render twice. With the old _middleware.js, there was no similar problem. Please note that if you rename or remove middleware.js from root, it works without any issues.

Expected Behavior

Pages render only once

Link to reproduction

https://github.com/imangm/nextjs-middleware-rerender

To Reproduce

  1. Clone the repo
  2. Run npm run build
  3. Run npm run start
  4. In Chrome DevTools, go to Network tab -> Network conditions
  5. Turn on Disable cache, also set Network throttling to Slow 3G to see the issue clearly
  6. Open the project in chrome
  7. Once the page loads, quickly scroll down, once the page is loaded once, it will load again and will automatically scroll to top
  8. In the console, you can see render _app is logged twice
  9. Also if you run the project using npm run dev, you can check the chrome console to see the re-render
  10. A live version of the repo is available here: https://next-middleware-rerender.vercel.app/
@imangm imangm added the bug Issue was opened via the bug report template. label Jul 3, 2022
@chozzz
Copy link
Contributor

chozzz commented Jul 3, 2022

I think my issue is similar to this. CMIIW if it's not, I will create a new issue.


However for me, the second render is going to my catch-all route, which is not the intended route I need to land on.

I have prepared an env to reproduce this;

  1. Access the following URL https://nextjs-q9mtiugm8-choz.vercel.app/demo/others/products/slug-1 (Which is a dynamic route, more details are logged in the console).
  2. You'll notice that for a split second, you're landed on dynamic route. But then, you're navigated to my catch-all route after.

Link to reproduction
https://github.com/chozzz/nextjs-poc/tree/issue-38267

[EDIT]:

  • The redirect issue (point 2 above) seemed to only happen after deploying to Vercel initially. It's no longer happening now for some reason (Probably because it's cached by edge? not sure - But, I've recorded it in a video below).
  • However, the page renders multiple times can still be seen in the log.
nextjs-poc-38267_1.mp4

@imangm
Copy link
Author

imangm commented Jul 3, 2022

I think my issue is similar to this. CMIIW if it's not, I will create a new issue.

However for me, the second render is going to my catch-all route, which is not the intended route I need to land on.

I have prepared an env to reproduce this;

  1. Access the following URL https://nextjs-brd84tmzz-choz.vercel.app/demo/others/products/slug-1 (more details in the console)
  2. You'll notice that you're landed on my catch-all route.

Link to reproduction https://github.com/chozzz/nextjs-poc/tree/issue/38267

@chozzz Yes exactly! On my main project that I noticed this issue, my app structure is as below:

/pages
/pages/index.js
/pages/[...catchall].js (it could include also /blog as a route that is created in CMS - I have the /blog in filesystem routing as well)
/pages/blog/[dynamic-route].js
/pages/nl/index.js
/pages/nl/[...catchall].js
/pages/nl/blog/[dynamic-route].js

What I noticed is that the main issue is pages are rendering twice, but at the same time, when you render a dynamic-route which has a catch-all-route on its parent (like dynamic routes in /blog/[uri].js), the dynamic-route page loads the first time using its own component, on second time, it re-render using the parent catch-all route also without passing the props.

What actually happens on those dynamic-route pages, is that the page loads completely without any issue (loads all content properly), but on re-render (that should not happen at the first place), it uses the catch-all route component without passing the props and the page becomes empty or returns this error based on whether accurate data validation is done or not:

Error: Invariant: attempted to hard navigate to the same URL

I wasn't able to re-produce it on the repo that I made to report this bug because I thought the problem is with the re-render thing, but I can confirm that the same thing happens for me too.

I hope that helps.

@ch3rn1k
Copy link

ch3rn1k commented Jul 3, 2022

Agree, same with my #38273 issue

@11koukou
Copy link
Contributor

11koukou commented Jul 4, 2022

Confirming, same here. It occurs in both production and development mode. I use getInitialProps in custom app and the devtools react profiler reported that the cause of the re-rendering was the change of props in _app.tsx . Disabling the middleware fixes the double render...

@willemliufdmg
Copy link

willemliufdmg commented Jul 4, 2022

I think this may be the cause of the performance issue I filed earlier:

@imangm
Copy link
Author

imangm commented Jul 4, 2022

I think this may be the cause of the performance issue I filed earlier:

@willemliufdmg IMHO, This issue could surely affect the performance but I think the one that you mentioned in #38235 could be different since it affects the server response time and TTFB. This one happens after the page is loaded.

[EDIT]

I think it's more relevant to #38171

@11koukou
Copy link
Contributor

11koukou commented Jul 6, 2022

This should be addressed with high priority

@Escapado
Copy link

Escapado commented Jul 6, 2022

We also noticed that behaviour and it stops us from migrating from 12.0 to 12.2. Also the pageProps are empty on the second render but not on the first and we use them extensively.

@imangm
Copy link
Author

imangm commented Jul 6, 2022

@Escapado Just curious to know, are you using catch-all routes + dynamic routes together? Because that's the exact same issue that I have and I wonder if besides double rendering, ignoring the props for the second render is because of this directory structure or if it's a common issue with new middleware in 12.2?

@chozzz
Copy link
Contributor

chozzz commented Jul 6, 2022

@imangm I am not sure if he has done the same like us, but I can assure you this just starts happening in 12.2 (for me at least). I confirm that 12.1.4 works fine for me.

@jescalan
Copy link
Contributor

jescalan commented Jul 6, 2022

We are looking into this on the nextjs team and it will be addressed with high priority. Thank you for all the background and discussion here everyone 🙏

@ijjk
Copy link
Member

ijjk commented Jul 6, 2022

Hi, to add some context here, the additional render when an SSG page matches middleware is currently expected as middleware can provide additional query values that the SSG page may be expecting, so along with the query updating process we already do for static pages (related docs) we also trigger this update when middleware matches.

We have investigated skipping the render when the router state has not changed (related PR) although there are edge cases that need to be investigated there for this optimization and an additional render should not cause performance issues unless expensive logic is blocking renders which can hurt performance regardless of this additional render.

@11koukou
Copy link
Contributor

11koukou commented Jul 6, 2022

We have investigated skipping the render when the router state has not changed (#37431) although there are edge cases that need to be investigated there for this optimization and an additional render should not cause performance issues unless expensive logic is blocking renders which can hurt performance regardless of this additional render.

@jjjjjjj
Thank you for your response, I am not sure about the extra re-render being an issue but I am sure that using the middleware (for just redirecting to the desired locale) drops my pagespeed score about ~7 points, bringing it often to the yellow zone. When not using it the score is constantly in the green zone especially after your great latest addition of the next/future/image optimization

@imangm
Copy link
Author

imangm commented Jul 6, 2022

@ijjk Thank you for checking this. However could you please confirm if this behavior was the same with the old _middleware or not? As I checked it wasn't like this and it was only rendering the pages once.

Also if you kindly check it after changing Network throttling to Slow 3G to slow down things, you'll notice that the page reloads completely. Please also check the scenario that using dynamic-routes that have a parent catch-all route because it re-renders the second time without passing the props and will render inside the component of the catch-all.

To be more clear, it loads the whole "dynamic route" file. Then refreshes the page with "Catch-all route" and doesn't send any props and the page will break.

Thanks again

@chozzz
Copy link
Contributor

chozzz commented Jul 7, 2022

Hi @ijjk the PR above adds a validation on top of the existing feature to prevent this from happening. What is the root cause for the issue? Is it possible to have the root cause fixed instead?
Sorry for preying it deeper, I don't mind with the fix. But, just being wary if there's a new issue introduced on top of the fix.

@shuding
Copy link
Member

shuding commented Jul 7, 2022

Hi all! I'm going to verify if it's safe to re-land #37431 and also add a test for this specific issue.

Hi @ijjk the PR above adds a validation on top of the existing feature to prevent this from happening. What is the root cause for the issue? Is it possible to have the root cause fixed instead?

The reason why it was happening in the first place is we can't easily tell if there's gonna be a query or not, when accessing to certain routes. If there's a query, a re-render to update the route state is necessary to avoid hydration mismatch issues.

@zackify
Copy link

zackify commented Jul 19, 2022

when you render a dynamic-route which has a catch-all-route on its parent (like dynamic routes in /blog/[uri].js), the dynamic-route page loads the first time using its own component, on second time, it re-render using the parent catch-all route also without passing the props.

I'm seeing the exact same thing. Vercel should be ashamed of this release. It has broken a lot of functionality and took a long time to figure out.

Basically if you have a middleware file, this is a problem. Routes with parent catch alls get randomly rendered on the client side. Shallow routing doesn't work. I even noticed a whole bunch of extra data loading methods for unrelated pages being done in the network tab. For now I will have to revert my app back a version...

@alexturpin
Copy link

Basically if you have a middleware file, this is a problem. Routes with parent catch alls get randomly rendered on the client side. Shallow routing doesn't work. I even noticed a whole bunch of extra data loading methods for unrelated pages being done in the network tab. For now I will have to revert my app back a version...

Same here, this behaviour breaking shallow routing sent me back to a previous version of Next. Pretty bad bug.

@11koukou
Copy link
Contributor

Any news from #38422 ?

@11koukou
Copy link
Contributor

Nothing changed with latest version :(

@kodiakhq kodiakhq bot closed this as completed in #38422 Jul 25, 2022
kodiakhq bot pushed a commit that referenced this issue Jul 25, 2022
Lands #37431 again, but it only solves the re-render issue completely for the middleware case (closes #38267), not the `rewrites` case (#37139).

For `rewrites`, the blocker is `isReady` always being `false` initially and to match the markup on hydration we can't simply work around it without a re-render. Need to come up with another fix.

## Bug

- [x] Related issues linked using `fixes #number`
- [x] Integration tests added
- [ ] Errors have helpful link attached, see `contributing.md`

## Feature

- [ ] Implements an existing feature request or RFC. Make sure the feature request has been accepted for implementation before opening a PR.
- [ ] Related issues linked using `fixes #number`
- [ ] Integration tests added
- [ ] Documentation added
- [ ] Telemetry added. In case of a feature if it's used or not.
- [ ] Errors have helpful link attached, see `contributing.md`

## Documentation / Examples

- [ ] Make sure the linting passes by running `pnpm lint`
- [ ] The examples guidelines are followed from [our contributing doc](https://github.com/vercel/next.js/blob/canary/contributing.md#adding-examples)
@11koukou
Copy link
Contributor

Thank you

@github-actions
Copy link
Contributor

This closed issue has been automatically locked because it had no new activity for a month. If you are running into a similar issue, please create a new issue with the steps to reproduce. Thank you.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 25, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Issue was opened via the bug report template.
Projects
None yet
Development

Successfully merging a pull request may close this issue.