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] Fix Netlify build cache issue #14182

Merged
merged 1 commit into from
Aug 13, 2024

Conversation

cherniavskii
Copy link
Member

Closes #14104

Netlify docs say the --shamefully-hoist flag is required for Next.js + pnpm setup, but as pointed out by @Janpot this guide doesn't apply to us since we build+export and just host a static site.

@cherniavskii cherniavskii added the core Infrastructure work going on behind the scenes label Aug 13, 2024
@cherniavskii cherniavskii requested a review from a team August 13, 2024 10:54
@cherniavskii cherniavskii added the scope: code-infra Specific to the core-infra product label Aug 13, 2024
@mui-bot
Copy link

mui-bot commented Aug 13, 2024

Deploy preview: https://deploy-preview-14182--material-ui-x.netlify.app/

Generated by 🚫 dangerJS against 1a4aa8f

Copy link
Member

@LukasTy LukasTy left a comment

Choose a reason for hiding this comment

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

LGTM. 👏
Thank you for all the effort on this matter! 🙏 💯

Copy link
Member

@MBilalShafi MBilalShafi left a comment

Choose a reason for hiding this comment

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

🎉

@cherniavskii cherniavskii merged commit fbed7b1 into mui:master Aug 13, 2024
22 checks passed
@cherniavskii cherniavskii deleted the fix-netlify-build-cache branch August 13, 2024 11:52
@michaldudak
Copy link
Member

Just for the record - Core needs this as it uses Netlify functions.

@cherniavskii
Copy link
Member Author

@michaldudak Hmm, I'm not sure using Netlify functions justifies the need for this flag, it seems to be required only when using Next.js/Nuxt + pnpm.

In certain scenarios, you must pass additional flags to the pnpm install command. For example, some frameworks such as Nuxt 3 and Next.js require that you modify the pnpm install command. To avoid import issues with pnpm and these frameworks, use the PNPM_FLAGS environment variable and set it to --shamefully-hoist.
https://docs.netlify.com/configure-builds/manage-dependencies/#pnpm

@LukasTy
Copy link
Member

LukasTy commented Aug 14, 2024

And to add salt to the injury—Core also had the same Netlify problems at the same time (not sure if it is still present on old PRs—probably is/would be). 🤔

@michaldudak
Copy link
Member

@michaldudak Hmm, I'm not sure using Netlify functions justifies the need for this flag, it seems to be required only when using Next.js/Nuxt + pnpm.

When I was migrating the Core repo to pnpm, I had issues with functions that were unable to find transitive dependencies.

@LukasTy
Copy link
Member

LukasTy commented Aug 16, 2024

When I was migrating the Core repo to pnpm, I had issues with functions that were unable to find transitive dependencies.

But now there are no transitive dependencies needed by functions AFAIK.
Maybe it was before they were moved to the top level package.json? 🤔

@michaldudak
Copy link
Member

If it works, I've got no objections to remove this flag :)

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 scope: code-infra Specific to the core-infra product
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants