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

[core] Replace getInitialProps with getStaticProps #33684

Merged

Conversation

mnajdova
Copy link
Member

Related to #33196 (comment)

We still need getInitialProps on the non-page files, like _document.js, _app.js etc.

@mui-bot
Copy link

mui-bot commented Jul 28, 2022

No bundle size changes

Generated by 🚫 dangerJS against 52caa28

@mnajdova mnajdova added docs Improvements or additions to the documentation core Infrastructure work going on behind the scenes and removed docs Improvements or additions to the documentation labels Jul 28, 2022
@mnajdova mnajdova changed the title [docs] Replace getInitialProps with getStaticProps [core] Replace getInitialProps with getStaticProps Jul 28, 2022
@mnajdova mnajdova marked this pull request as ready for review July 28, 2022 14:24
@mnajdova mnajdova requested review from Janpot and a team July 28, 2022 14:24
Copy link
Member

@Janpot Janpot left a comment

Choose a reason for hiding this comment

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

A lot of these pages are almost duplicates, I wonder if they can be deduplicated using getStaticPaths and have them generated in the export phase instead of the compilation phase. I wonder whether it would speed up the build since less pages will have to be compiled.

@eps1lon
Copy link
Member

eps1lon commented Jul 28, 2022

I couldn't make this work in the past for all locales. So I would recommend also running export for all locales.

@Janpot
Copy link
Member

Janpot commented Jul 29, 2022

Yep, using require.context wouldn't be possible in that scenario. It'd need to be replaced with regular filesystem access. Was just mentioning getStaticPaths as it seemed to be the obvious choice for implementing this. Not having tried this, I'm probably overlooking some more potential blockers.

@eps1lon
Copy link
Member

eps1lon commented Jul 29, 2022

I tried it multiple times but never took the time writing down why. It all came down to vervel considering our use case exotic and not helping with guidance etc. It might work now and I would definitely recommend doing it long term. Just be sure to test how master would be deployed not just branches. And test /api and other routes separately.

@mnajdova mnajdova merged commit 2b2b9c4 into mui:master Aug 1, 2022
@oliviertassinari
Copy link
Member

oliviertassinari commented Aug 13, 2022

I couldn't make this work in the past for all locales. So I would recommend also running export for all locales.

@mnajdova The point of Sebastian is confirmed. This PR broke the SEO structure when it comes to the translations. For example, the page https://mui.com/material-ui/api/table-pagination/ says:

<link rel="alternate" href="https://mui.com/zh/material-ui/api/table-pagination/" hrefLang="zh"/>

But this page is a 404 https://mui.com/zh/material-ui/api/table-pagination/. Chinese users will directly get a 404.

The Google Search console started to raise flags about it (how I landed here). I could find a bit of documentation about it on Next.js's side: vercel/next.js#15315 (comment).


What I would propose is to:

  1. Fix the regression. Revert this PR (trading a deprecation for a working SEO structure sounds like a good deal). A quick win. The sooner, the better.
  2. Have a look at https://nextjs.org/docs/basic-features/data-fetching/get-static-paths as suggested by Jan. It seems that we could get a nice speed up, same for the blog pages.

Otherwise:

  1. Fix the regression. The sooner, the better.
    a. Add 301 for all the URLs that are now 404
    b. Remove the alternate hrefLang
    c. Remove the automatic redirection to these missing URL
  2. Do point 2. of the proposal above.

@oliviertassinari oliviertassinari added the docs Improvements or additions to the documentation label Aug 13, 2022
@mnajdova
Copy link
Member Author

I don't really have bandwidth to look into this more. I can revert the PR and delegate to someone else on the team to handle.

@oliviertassinari
Copy link
Member

oliviertassinari commented Aug 19, 2022

delegate to someone else on the team to handle

@mnajdova Ok, sounds great. It will likely be for either:

  1. once we need to upgrade Next.js
  2. or once we move forward with the implementation of https://mui-org.notion.site/Docs-localization-what-s-next-3f370280b58f446fbb5559dfc08fe03e (considering we are heading for no translations in production). In this effort, we would need to change SEO structure too.

daniel-rabe pushed a commit to daniel-rabe/material-ui that referenced this pull request Nov 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Infrastructure work going on behind the scenes docs Improvements or additions to the documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants