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

[docs] Fix Next.js hosting on mui.com #661

Merged
merged 12 commits into from
Jul 22, 2022
Merged

[docs] Fix Next.js hosting on mui.com #661

merged 12 commits into from
Jul 22, 2022

Conversation

bharatkashyap
Copy link
Member

@bharatkashyap bharatkashyap commented Jul 15, 2022

@render
Copy link

render bot commented Jul 15, 2022

@oliviertassinari oliviertassinari requested a deployment to redirect-fix - toolpad-db PR #661 July 15, 2022 14:31 — with Render Abandoned
@oliviertassinari oliviertassinari changed the title [docs] Landing page redirect fix [docs] Fix Next.js hosting on mui.com Jul 15, 2022
@oliviertassinari oliviertassinari added docs Improvements or additions to the documentation bug 🐛 Something doesn't work labels Jul 15, 2022
Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

this doesn't seem to work, the assets are still hosted under the conflicting folder:
https://deploy-preview-661--mui-toolpad-docs.netlify.app/_next/static/chunks/871-bbf549ab618193c5.js. We have only customized the build folder location, not the URLs.

@@ -141,6 +141,7 @@ module.exports = withTM({
return map;
},
reactStrictMode,
distDir: 'dist',
Copy link
Member

Choose a reason for hiding this comment

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

Based on the nature of the problem that we are trying to solve: scoping. I think that we should bring more context. Maybe something like this:

Suggested change
distDir: 'dist',
distDir: '_next/toolpad',

Copy link
Member

Choose a reason for hiding this comment

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

I think the basePath option is meant for exactly this purpose, scoping.

Copy link
Member

@oliviertassinari oliviertassinari Jul 15, 2022

Choose a reason for hiding this comment

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

I think the basePath option is meant for exactly this purpose, scoping.

I had a quick look at this in vercel/next.js#31718 (comment)

@oliviertassinari
Copy link
Member

oliviertassinari commented Jul 15, 2022

I suspect that we need assetPrefix: isProduction ? '/toolpad/_next' : '' which in https://github.com/mui/material-ui/blob/62a777e6ea4dd13c5e9f369b6c843a389b160dda/docs/public/_redirects#L401 would need:

/toolpad/_next/* https://mui-toolpad-docs.netlify.app/_next/:splat 200

It's not awesome because it would mean that the previews in the production deploy would be broken https://app.netlify.com/sites/mui-toolpad-docs/overview. I couldn't find anything better to make it work in production. I have commented in #660 what I have explored.

@render
Copy link

render bot commented Jul 21, 2022

@oliviertassinari oliviertassinari requested a deployment to redirect-fix - toolpad-db PR #661 July 21, 2022 11:46 — with Render Abandoned
@bharatkashyap
Copy link
Member Author

bharatkashyap commented Jul 21, 2022

b46dc1e

Using the NETLIFY environment variable to differentiate when to add the assetPrefix, along with a redirect rule from /toolpad/_next to /_next in the docs folder for the Netlify site seems to do the trick, as the latest preview shows

@oliviertassinari
Copy link
Member

oliviertassinari commented Jul 21, 2022

@bharatkashyap Ah smart. 👍 from my side, we avoid the limitation of point 3 in #660.


I think that we have two steps left:

@bharatkashyap
Copy link
Member Author

bharatkashyap commented Jul 22, 2022

To clean up this PR, I think that we should revert the change of the build folder from docs/.next to docs/dist it doesn't help.

Yes, have done that

/toolpad/_next/* https://mui-toolpad-docs.netlify.app/_next/:splat 200

mui/material-ui#33524

Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

👌

docs/public/_redirects Show resolved Hide resolved
docs/next.config.js Show resolved Hide resolved
bharatkashyap and others added 2 commits July 22, 2022 03:46
Co-authored-by: Olivier Tassinari <[email protected]>
Co-authored-by: Olivier Tassinari <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work docs Improvements or additions to the documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants