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

[examples] Next.js v13 use legacy behavior to fix example #34970

Merged
merged 5 commits into from
Nov 2, 2022

Conversation

PetroSilenius
Copy link
Contributor

@PetroSilenius PetroSilenius commented Oct 31, 2022

Background 📚

Relates to #34898

In Next.js v13 Link component now renders a a tag and does not allow a tag as a child.

Next.js v13 release blog post
Next/link docs

This is a breaking change and currently causes errors in the next.js examples. Aim of this pull request is to fix those breaking changes and simplifying the examples could be done in a separate pull request which might need some more discussion.

Changes 🕹

  • Adds support for legacyBehaviour prop in NextLinkComposed and Link components in the next.js examples repos

  • Makes the legacyBehaviour prop true` by default so it's compatible with the current implementation

  • I have followed (at least) the PR section of the contributing guide.

Fixes #34893

@mui-bot
Copy link

mui-bot commented Oct 31, 2022

Messages
📖 Netlify deploy preview: https://deploy-preview-34970--material-ui.netlify.app/

No bundle size changes

Generated by 🚫 dangerJS against d24f393

@zannager zannager added the examples Relating to /examples label Nov 1, 2022
@zannager zannager requested a review from mnajdova November 1, 2022 05:58
Copy link
Member

@mnajdova mnajdova left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution, I was planning to tackle this today :) The changes look good, I've tested them locally. This seems to fix #34893, the server-side rendering looks good.

@mnajdova mnajdova requested a review from siriwatknp November 1, 2022 08:25
@mnajdova
Copy link
Member

mnajdova commented Nov 1, 2022

Tagging @siriwatknp for a final review.

@PetroSilenius
Copy link
Contributor Author

Ah totally missed #34893 but good that I was able to help!

I wrote down some thoughts about a proper migration as well here but would require support for both v12 and v13

@mnajdova
Copy link
Member

mnajdova commented Nov 1, 2022

I wrote down some thoughts about a proper migration as well #34898 (comment) but would require support for both v12 and v13

Makes sense. At this moment we have only one next.js example that depends on latest (which at this moment is next.js 13). Maybe we will need to create two examples for both next.js 12 and 13.

@mnajdova mnajdova merged commit 429404b into mui:master Nov 2, 2022
@PetroSilenius PetroSilenius deleted the nextjs-example-v13-links branch November 2, 2022 14:15
@oliviertassinari oliviertassinari added the component: link This is the name of the generic UI component, not the React module! label Nov 2, 2022
@oliviertassinari
Copy link
Member

oliviertassinari commented Nov 2, 2022

at this moment we have only one next.js example that depends on latest

Speaking on dependency on latest, this looks like we should depend on latest, 18 was released some time ago 😁

From what I understand, developers can easily migrate from Next.js v12 to v13: https://nextjs.org/blog/next-13#breaking-changes. So if we assume that most people looking at our example are starting a new project, I think that v13 would be good enough.

But if we were publishing a Next.js dedicated component, I think that supporting both would have been important.

@madaher-dev
Copy link

I see many issues open regarding next13 migration and they are labeled as completed and merge and people are saying nice work and stuff but couldn't see any working example without _document.js
am i missing something?

daniel-rabe pushed a commit to daniel-rabe/material-ui that referenced this pull request Nov 29, 2022
@Yedidya10
Copy link

I ran into a weird error.
I implemented the example in the js version and the index file is displayed well. But when I click the link to the about page I am automatically sent to http://localhost:3000/about-2 and a 404 page is displayed.
Does anyone know a solution to this?

I'm using next13

@madaher-dev
Copy link

at this moment we have only one next.js example that depends on latest

Speaking on dependency on latest, this looks like we should depend on latest, 18 was released some time ago 😁

From what I understand, developers can easily migrate from Next.js v12 to v13: https://nextjs.org/blog/next-13#breaking-changes. So if we assume that most people looking at our example are starting a new project, I think that v13 would be good enough.

But if we were publishing a Next.js dedicated component, I think that supporting both would have been important.

next13 still have many issues in the app directory. many people are not upgrading and are sticking to 12 until 13 is stable. so having two examples would really help.

@oliviertassinari
Copy link
Member

@madaher-dev Why not use Next.js v13 with the stable pages folder (rather than the beta app folder or Next.js v12)?

@madaher-dev
Copy link

@madaher-dev Why not use Next.js v13 with the stable pages folder (rather than the beta app folder or Next.js v12)?

Because the app/folder pages is not the only hold back in a next 13 migration. For example you still cant setCookie as cookies-next is not supported in 13. There are some issue also with importing script with a windows environment etc. Next 13 is a great concept but launching it in the way they did was premature imho

feliperli pushed a commit to jesrodri/material-ui that referenced this pull request Dec 6, 2022
@oliviertassinari oliviertassinari changed the title [examples] Next.js examples v13 - links [examples] Next.js v13 use legacy behavior to fix example Jan 25, 2023
@oliviertassinari oliviertassinari added external dependency Blocked by external dependency, we can’t do anything about it bug 🐛 Something doesn't work labels Jan 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work component: link This is the name of the generic UI component, not the React module! examples Relating to /examples external dependency Blocked by external dependency, we can’t do anything about it nextjs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Next.js 13] Hydration failed because the initial UI does not match what was rendered on the server
9 participants