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

[code-infra] Refactor Netlify cache-docs plugin setup #14105

Merged
merged 12 commits into from
Aug 9, 2024

Conversation

LukasTy
Copy link
Member

@LukasTy LukasTy commented Aug 5, 2024

We are rocking a monorepo and we deploy our documentation Netlify, which is on docs folder to netlify.

Based on their doc and the following points from our setup might not be the most correct and it could be resulting in flaky cache behavior. 🤔

  1. If you are using yarn workspaces 14 or a monorepo 16 please consult the appropriate documentation (linked to in this sentence).
    ~ 6. Fairly rare but worth mentioning: You have a package.json in your repo root, but you are building a subdirectory of your repo and have NOT appropriately set the base directory in your build settings. That can lead to some confusing caching situations since we’ll only sometimes use that package.json (when there is nocache).

Do you see any problems with it?
Is it worth testing it?

Given this doc my initial assumption was incorrect.


Why I tried looking in this?

A recent PR (#14092) failed Netlify deployment.


The final result of this PR is just some refactoring as the initial agenda seemed incorrect given our current setup.

  1. Refactor netlify-plugin-cache-docs plugin management by re-exporting it from the internal package and target it, instead of @mui/monorepo in the netlify.toml.
  2. Use native Node fetch instead of node-fetch in deploy-succeeded Netlify function.

@LukasTy LukasTy added the scope: code-infra Specific to the core-infra product label Aug 5, 2024
@LukasTy LukasTy self-assigned this Aug 5, 2024
netlify.toml Outdated Show resolved Hide resolved
@mui-bot
Copy link

mui-bot commented Aug 5, 2024

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

Generated by 🚫 dangerJS against de065b6

@LukasTy LukasTy requested review from cherniavskii, a team and JCQuintas August 5, 2024 15:02
@LukasTy LukasTy marked this pull request as ready for review August 5, 2024 15:02

[build.environment]
NODE_VERSION = "18"
NODE_OPTIONS = "--max_old_space_size=4096"
PNPM_FLAGS = "--shamefully-hoist"

[[plugins]]
package = "./node_modules/@mui/monorepo/packages/netlify-plugin-cache-docs"
Copy link
Member

@Janpot Janpot Aug 5, 2024

Choose a reason for hiding this comment

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

🤔 Out of scope, but we should try the same trick as we do with eslint-plugin-material-ui instead of accessing node_modules directly

Copy link
Member Author

Choose a reason for hiding this comment

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

I've tried a minimal setup with the main requirements—manifest file and re-exporting the @mui/monorepo package.
WDYT? 🤔

netlify.toml Outdated Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

Have you tested if this solves the Module not found: Can't resolve '@mui/joy/InitColorSchemeScript' issue?

Copy link
Member Author

Choose a reason for hiding this comment

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

I was under the impression that there is nothing to test in that regard.
The issue is the same that we had before bumping @mui/material to unblock the bump of @mui/monorepo and is repeated when a stale cache is picked up. 🙈
Do you have reasons to believe that the culprit of the failure are different? 🤔

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, I forgot about that one.

Do you have reasons to believe that the culprit of the failure are different? 🤔

I'm not sure it's only about caching, because we do run pnpm install after the cache is restored.
So it shouldn't matter if the cache is stale, it should be invalidated and dependencies should be properly resolved.
Right?

I was looking for the problem on our side, and this is why I opened #14104. But perhaps this was just a coincidence, I'm confused now 😅

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure it's only about caching, because we do run pnpm install after the cache is restored.
So it shouldn't matter if the cache is stale, it should be invalidated and dependencies should be properly resolved.
Right?

I was looking for the problem on our side, and this is why I opened #14104. But perhaps this was just a coincidence, I'm confused now 😅

If only mui-x was having problems, I would have also investigated possible problems on our end.
But the problem is that both material-ui and mui-x started having the same problems at the same time. 🤔

Copy link
Member

Choose a reason for hiding this comment

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

I still don't understand how stale cache can survive pnpm install though 🤔

Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should try downgrading pnpm?

Copy link
Member Author

Choose a reason for hiding this comment

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

I still don't understand how stale cache can survive pnpm install though 🤔

Netlify caches everything.
Maybe the installation problem gets fixed after running pnpm install, but the build files (.next directory) have also been saved and could make the build fail depending on how Netlify manages this case... 🙈
WDYT, does that sound feasible?

We could consider no longer caching the .next folder, but that would slow down many deploys significantly. 🙈 🤷

Copy link
Member Author

Choose a reason for hiding this comment

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

I've tested the build both with docs/.next caching and without and it seems like the difference in build time is somewhat negligible. 🙈
Screenshot 2024-08-07 at 12 03 04
1st build is without cache, the second is with it...
WDYT @mui/code-infra, does it make sense to additionally cache docs/.next? 🤔

Copy link
Member

@Janpot Janpot Aug 7, 2024

Choose a reason for hiding this comment

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

I tried the same last week in my local setup and the difference in next build time seemed to be about 1 to 2 ratio. (time pnpm docs:build with and without .next folder present)
The build step (next build) in both your builds takes about 4-5 minutes. I looked at a random other X netlify build and the build step is about 2-3 minutes. Are you sure the one you say is using cache is actually using cache?

(Also, we can probably just cache .next/cache instead of the whole .next folder)

Two more options I'd consider trying:

  • downgrade pnpm 1 or 2 minor versions (or any version we were on before the trouble started)
  • contact netlify support

edit: best to look at actual build times in the logs. There will be less variation. I think the Netlify overall build time includes queueing as well

Copy link
Member

Choose a reason for hiding this comment

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

We could consider no longer caching the .next folder

I've tried that, you can find it in the "Disproved theories" section in #14104

@LukasTy LukasTy changed the title [code-infra] Set explicit Netlify base folder [code-infra] Refactor Netlify docs-cache plugin management Aug 7, 2024
@LukasTy LukasTy changed the title [code-infra] Refactor Netlify docs-cache plugin management [code-infra] Refactor Netlify cache-docs plugin management Aug 7, 2024
@LukasTy LukasTy changed the title [code-infra] Refactor Netlify cache-docs plugin management [code-infra] Refactor Netlify cache-docs plugin setup Aug 7, 2024
@@ -1,5 +1,3 @@
const fetch = require('node-fetch');
Copy link
Member

Choose a reason for hiding this comment

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

ok, this wasn't even in package.json? 😄

Copy link
Member Author

Choose a reason for hiding this comment

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

Nope. 🙈
We relied on a transitive dependency. 😆

@LukasTy LukasTy merged commit 5ad5cf3 into mui:master Aug 9, 2024
18 checks passed
@LukasTy LukasTy deleted the play-with-netlify branch August 9, 2024 13:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
scope: code-infra Specific to the core-infra product
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants