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] Re-add nx and setup build caching #38752

Merged
merged 1 commit into from
Sep 5, 2023

Conversation

brijeshb42
Copy link
Contributor

First run of release:build
Screenshot 2023-09-01 at 11 03 10 AM

Subsequent run of release:build
Screenshot 2023-09-01 at 11 03 36 AM

nx is especially helpful to me while I am working on zero-runtime packages as they can't be consumed directly as of now. They need to be built everytime before consumption in their respective apps.

@brijeshb42 brijeshb42 added the core Infrastructure work going on behind the scenes label Sep 1, 2023
@mui-bot
Copy link

mui-bot commented Sep 1, 2023

Netlify deploy preview

https://deploy-preview-38752--material-ui.netlify.app/

Bundle size report

No bundle size changes (Toolpad)
No bundle size changes

Generated by 🚫 dangerJS against 7ec9004

@brijeshb42 brijeshb42 requested review from a team and removed request for michaldudak and Janpot September 1, 2023 05:40
@michaldudak
Copy link
Member

michaldudak commented Sep 1, 2023

Since the build tasks are used to prepare the packages to release, I'd prefer to be on the safe side and do a fresh build without cache then. I understand you have a different use case, so perhaps we could introduce the rebuild tasks (which would be triggered by the root release:build) that would clean the output and not use any cache. Then we could have the build task in the root, triggering the cacheable build tasks from individual projects. cc @Janpot

@brijeshb42
Copy link
Contributor Author

Ok. I've updated release:build to not use cache and added a build command that'll use cache by default.

@brijeshb42 brijeshb42 force-pushed the feat/readd-nx branch 2 times, most recently from d81ab26 to 882bc45 Compare September 1, 2023 07:50
lerna.json Outdated Show resolved Hide resolved
nx.json Show resolved Hide resolved
@Janpot
Copy link
Member

Janpot commented Sep 1, 2023

You can run npx lerna repair to verify the setup.

Since the build tasks are used to prepare the packages to release, I'd prefer to be on the safe side and do a fresh build without cache then.

Agreed, maybe once we're publishing from CI we can think about enabling caching agin?

There has been some exploration around introducing a task runner to set up dependencies between scripts:

I'll create a shaping page on notion to collect feedback across teams

@oliviertassinari
Copy link
Member

oliviertassinari commented Sep 2, 2023

helpful to me while I am working on zero-runtime packages as they can't be consumed directly as of now. They need to be built everytime before consumption in their respective apps.

Why not solve this with a build watch mode? This sounds like it would be faster in dev mode, and more deterministic than using nx cache.

@brijeshb42
Copy link
Contributor Author

I can try adding a watch mode if nx has one. That way it'll continously build the changed package. But the flow still remains same mainly because I need to import items in the config file itself (vite/webpack) which doesn't work directly. So in the consumer app, I still need to do yarn install to get the changes.

@oliviertassinari
Copy link
Member

oliviertassinari commented Sep 2, 2023

I can try adding a watch mode if nx has one.

@brijeshb42 I was thinking of vite build watch mode vitejs/vite#1434 (not nx).

Also, I guess we could bypass the build as we do it for the rest of the docs by consuming the source directly. I usually default to doing this out of simplicity, e.g. to not have to think of build until it's for releasing npm packages.

@oliviertassinari oliviertassinari changed the title [core] re-add nx and setup build caching [core] Re-add nx and setup build caching Sep 2, 2023
@oliviertassinari oliviertassinari added the package: system Specific to @mui/system label Sep 2, 2023
@brijeshb42
Copy link
Contributor Author

brijeshb42 commented Sep 2, 2023

The problem is that vite watch mode will only work on source files, not for the vite config file itself where we import the theme from material and pass it to our plugin. It would've worked perfectly fine if this import was in any of the app files.

In the case of docs, our setup won't work if we were to import something in the next config file.

@Janpot
Copy link
Member

Janpot commented Sep 4, 2023

Indeed, the way our dev mode in the docs works is by aliasing the dependent workspaces to their source files. The aliasing happens in the Next.js app compilation and doesn't apply to the next.config.js. yarn workspace linking doesn't work for us because our package.json files don't have module resolution fields set up (that gets added during the build in the build folder which is not a workspace).

@brijeshb42 From your comments I deduce that your current DX on this package is as follows: You have a small test project, in which you've linked the <path-to-material-ui-project>/packages/zero-runtime-package/build folder. After you make changes to the package you rebuild the whole monorepo and restart your project. Your goal with this PR is to shorten that build time by omitting workspaces that haven't changed. Is that correct?

@brijeshb42
Copy link
Contributor Author

@Janpot You are exactly right. That is the reason for this PR

@Janpot
Copy link
Member

Janpot commented Sep 4, 2023

Ok, then I think the approach could indeed be to limit which packages are being built. Have you tried scoping down the command to only one workspace?

yarn lerna run --scope @mui/zero-runtime-css build

or

yarn workspace @mui/zero-runtime-css run build

I don't want to block this PR if there is no alternative, but my personal preference would also be to wait with caching if we have other options. If we end up setting up nx/turbo I think we should initially concentrate on orchestrating the different scripts more efficiently, rather than the caching. That's where we will gain the most performance (if any) in CI without setting up their cloud.

@brijeshb42
Copy link
Contributor Author

brijeshb42 commented Sep 4, 2023

I did have a build:zero command to build only zero-runtime related packages but even then, the problem was that there are multiple zero packages and sometimes all might change and sometimes only 1. I didn't want to track and only build that package. That's why this PR.
To answer your concerns, the current release flow doesn't use cache. I have only added extra command along with nx.json so that I can use that command along with nx cache.

@brijeshb42 brijeshb42 removed the request for review from Janpot September 4, 2023 15:44
@brijeshb42 brijeshb42 requested a review from Janpot September 4, 2023 15:44
Copy link
Member

@Janpot Janpot left a comment

Choose a reason for hiding this comment

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

Ok for me 👍

@brijeshb42
Copy link
Contributor Author

If anyone else doesn't have any objections, I'll merge this tomorrow before release PR.

@brijeshb42 brijeshb42 merged commit 04b4d98 into mui:master Sep 5, 2023
9 checks passed
@brijeshb42 brijeshb42 deleted the feat/readd-nx branch September 5, 2023 08:50
mnajdova pushed a commit to mnajdova/material-ui that referenced this pull request Sep 8, 2023
mnajdova pushed a commit to mnajdova/material-ui that referenced this pull request Sep 8, 2023
xcode-it pushed a commit to xcode-it/material-ui that referenced this pull request Sep 11, 2023
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 package: system Specific to @mui/system
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants