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.

Sign up for GitHub

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
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
753d32b
[code-infra] Set explicit Netlify `base` folder
LukasTy Aug 5, 2024
cf0ddb6
test paths
LukasTy Aug 5, 2024
f7a1d26
Use `docs` package command
LukasTy Aug 5, 2024
2b97a5e
Move netlify functions relate to the new `base`
LukasTy Aug 5, 2024
38bcc9e
Add ESLint ignore
LukasTy Aug 5, 2024
64a6319
Revert `base` directory changes
LukasTy Aug 7, 2024
d8091e0
Try using re-exported `netlify-plugin-cache-docs`
LukasTy Aug 7, 2024
59d8160
Add plugin manifest
LukasTy Aug 7, 2024
89f97fd
Remove no longer necessary comment
LukasTy Aug 7, 2024
1b534f5
Try without `docs/.next` caching
LukasTy Aug 7, 2024
c9efc1a
Use native node fetch
LukasTy Aug 7, 2024
de065b6
Bring back the cache plugin
LukasTy Aug 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion netlify.toml
View file
Open in desktop
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 •
edited
Loading

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

Original file line number Diff line number Diff line change
Expand Up @@ -13,4 +13,4 @@
PNPM_FLAGS = "--shamefully-hoist"

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

@Janpot Janpot Aug 5, 2024 •
edited
Loading

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? 🤔

package = "./packages/netlify-plugin-cache-docs"
2 changes: 0 additions & 2 deletions netlify/functions/deploy-succeeded.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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. 😆


/**
* @param {object} event
* @param {string} event.body - https://jsoneditoronline.org/#left=cloud.fb1a4fa30a4f475fa6887071c682e2c1
Expand Down
1 change: 1 addition & 0 deletions packages/netlify-plugin-cache-docs/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
module.exports = require('@mui/monorepo/packages/netlify-plugin-cache-docs');
1 change: 1 addition & 0 deletions packages/netlify-plugin-cache-docs/manifest.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
name: netlify-plugin-cache-docs