[core] Re-use latest code infra scripts from core repo #1662

brijeshb42 · 2025-04-01T18:18:19Z

Removed all the local code wherever possible and used scripts from the core repo.

I have followed (at least) the PR section of the contributing guide.

netlify · 2025-04-02T06:26:48Z

✅ Deploy Preview for base-ui ready!

Name	Link
🔨 Latest commit	`f4f8757`
🔍 Latest deploy log	https://app.netlify.com/sites/base-ui/deploys/67ecd7f1f2d3cc0008dc4c73
😎 Deploy Preview	https://deploy-preview-1662--base-ui.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

netlify · 2025-04-02T06:29:34Z

✅ Deploy Preview for base-ui ready!

Name	Link
🔨 Latest commit	`8a21680`
🔍 Latest deploy log	https://app.netlify.com/sites/base-ui/deploys/67f3832ca3f15a00083bf2c9
😎 Deploy Preview	https://deploy-preview-1662--base-ui.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Janpot · 2025-04-03T07:31:22Z

scripts/testBuiltTypes.mjs

-  console.error(error);
-  process.exit(1);
-});
+import '@mui/monorepo/scripts/testBuiltTypes.mjs';


Can keep it for now, but looking at the script, I assume this is unnecessary if your declarations are built from typescript files.

Actually, it's looking for mui-* segments in the paths, which would never occur in the base ui repo.

Actually, it's looking for mui-* segments in the paths

True. I wonder if it was even working as intended.

Janpot · 2025-04-03T07:38:31Z

scripts/useReactVersion.mjs

@@ -37,7 +37,9 @@ async function main(version) {
    return;
  }

-  const packageJsonPath = path.resolve(getWorkspaceRoot(), 'package.json');
+  const currentDirectory = url.fileURLToPath(new URL('.', import.meta.url));
+  const workspaceRoot = path.resolve(currentDirectory, '..');


Can't this script just default to process.cwd() and add the script to root package.jsons scripts. Then it will always run with cwd of the root, regardless of whether it's executed from a subfolder.

My personal preference is not to include everything in package.json scripts to avoid cluttering them, especially if the script is only used by the CI.

Janpot · 2025-04-03T07:39:58Z

scripts/validateTypescriptDeclarations.mts

-  });
-  const invalidFiles = declarationFiles.filter((file) => {
-    const content = fs.readFileSync(file, 'utf8');
-    const regex = /import\(["']packages\//gm;


🤔 I feel like this is trying to do the exact same thing as testBuiltTypes. And probably as irrelevant when declarations are generated from ts instead of handwritten.

This script was created to prevent issues such as mui/material-ui#33321 (caused by referencing a type that's not exported).

🤔 I'm suspecting the root cause of that issue is the use of baseUrl. We should try if we can still reproduce this bug with that option removed.

Janpot

Personally, I would remove the getWorkspaceRoot logic in favor for process.cwd(). Then never run the script directly, but always through a package.json script. That makes this script more portable within and across projects.

Looks great otherwise, I have a feeling some of those type validation scripts are a bit unnecessary.

package.json

packages/react/package.json

brijeshb42 · 2025-04-03T09:04:50Z

That makes this script more portable within and across projects.

By portable, you mean copy pastable to new repos in any directory, right ?

Janpot · 2025-04-03T09:47:51Z

By portable, you mean copy pastable to new repos in any directory, right ?

No, I mean the location in the file system of the script makes no difference on whether it behaves correctly. So we can simply move it to a published package and make it available in the path with package.json bin and it'll still work. I don't think we should encourage copy/pasting these tools.

brijeshb42 · 2025-04-03T09:54:40Z

The way this particular script works right now, it'll need to be copy pasted unless we change that.
Rest can be used from an installable package.

Janpot

Great job. I can't reproduce any problem that would break the types validation functions. @michaldudak for a final approval.

michaldudak · 2025-04-07T08:17:27Z

scripts/useReactVersion.mjs

@@ -37,7 +36,8 @@ async function main(version) {
    return;
  }

-  const packageJsonPath = path.resolve(getWorkspaceRoot(), 'package.json');
+  const workspaceRoot = process.cwd();


Now we're in a state where we must run the script from the root directory and don't have it added to npm scripts to enforce the correct working directory. My preference would be to leave the previous version, but if you and @Janpot like reading process.cwd and calling the script from npm, let's have it your way.

Wouldn't this script always have to run from the root directory anyways given it needs to modify root package.json
And the current invocation of script will remain as is even if it is added to package.json scripts.
Not sure I fully understand.

but if you and @Janpot like reading process.cwd and calling the script from npm, let's have it your way.

FYI: I don't really have some bizarre fetish to see process.cwd() 😄. My preference would be that we centralize this script instead of copying it over multiple repositories. In that scenario, the getWorkspaceRoot() as implemented in this repo would never work as it would exist in a different location relative to the workspace root (such as in the node_modules folder). Any CLI tool we use operates on the current working dir, or its parents. That's why I advocate for not writing our scripts in a way that its location in the file system matters. That way we make it easy for them to be distributed across the company.

Personally, for this script, I think it's a bit overkill to look for a root, but as always, code infra can only work when the project teams buy in to the idea. So if there is a strong desire for the functionality we can certainly add it back, but I believe the correct implementation should be to start from process.cwd() and progressively check parent folders for a pnpm-workspace.yaml file.

brijeshb42 added core Infrastructure work going on behind the scenes scope: code-infra Specific to the code-infra product labels Apr 1, 2025

brijeshb42 force-pushed the code-infra-core branch 7 times, most recently from f4f8757 to b1f0f5e Compare April 2, 2025 06:24

brijeshb42 mentioned this pull request Apr 2, 2025

[core] Update scripts to support base-ui mui/material-ui#45784

Merged

1 task

brijeshb42 force-pushed the code-infra-core branch 4 times, most recently from d73ca0c to eb0c574 Compare April 2, 2025 15:05

brijeshb42 marked this pull request as ready for review April 2, 2025 15:10

brijeshb42 requested review from michaldudak, atomiks and colmtuite as code owners April 2, 2025 15:10

brijeshb42 requested review from Janpot and a team and removed request for colmtuite, atomiks, michaldudak and Janpot April 2, 2025 15:10

brijeshb42 force-pushed the code-infra-core branch 2 times, most recently from fd5c639 to 68de990 Compare April 2, 2025 18:22

Janpot reviewed Apr 3, 2025

View reviewed changes

michaldudak reviewed Apr 3, 2025

View reviewed changes

package.json Outdated Show resolved Hide resolved

michaldudak reviewed Apr 3, 2025

View reviewed changes

packages/react/package.json Show resolved Hide resolved

brijeshb42 force-pushed the code-infra-core branch 2 times, most recently from 48e9447 to ad27d41 Compare April 3, 2025 08:59

brijeshb42 force-pushed the code-infra-core branch from ad27d41 to 44c7614 Compare April 4, 2025 13:14

brijeshb42 requested review from michaldudak and Janpot April 4, 2025 13:14

github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Apr 4, 2025

Janpot reviewed Apr 4, 2025

View reviewed changes

brijeshb42 force-pushed the code-infra-core branch from 44c7614 to 23819b6 Compare April 4, 2025 15:16

github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Apr 4, 2025

brijeshb42 force-pushed the code-infra-core branch from 23819b6 to 65fdda6 Compare April 4, 2025 15:22

Brijesh Bittu added 2 commits April 7, 2025 13:17

[core] Re-use latest code infra scripts from core repo

380d616

Move attw package to workspace

8a21680

brijeshb42 force-pushed the code-infra-core branch from 65fdda6 to 8a21680 Compare April 7, 2025 07:47

michaldudak reviewed Apr 7, 2025

View reviewed changes

github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Apr 11, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[core] Re-use latest code infra scripts from core repo #1662

[core] Re-use latest code infra scripts from core repo #1662

brijeshb42 commented Apr 1, 2025 •

edited

Loading

netlify bot commented Apr 2, 2025

netlify bot commented Apr 2, 2025 •

edited

Loading

Janpot Apr 3, 2025 •

edited

Loading

brijeshb42 Apr 3, 2025 •

edited

Loading

Janpot Apr 3, 2025

michaldudak Apr 3, 2025

Janpot Apr 3, 2025 •

edited

Loading

michaldudak Apr 3, 2025

Janpot Apr 3, 2025

Janpot left a comment •

edited

Loading

brijeshb42 commented Apr 3, 2025 •

edited

Loading

Janpot commented Apr 3, 2025

brijeshb42 commented Apr 3, 2025

Janpot left a comment •

edited

Loading

michaldudak Apr 7, 2025

brijeshb42 Apr 7, 2025

Janpot Apr 7, 2025

[core] Re-use latest code infra scripts from core repo #1662

Are you sure you want to change the base?

[core] Re-use latest code infra scripts from core repo #1662

Conversation

brijeshb42 commented Apr 1, 2025 • edited Loading

netlify bot commented Apr 2, 2025

✅ Deploy Preview for base-ui ready!

netlify bot commented Apr 2, 2025 • edited Loading

✅ Deploy Preview for base-ui ready!

Janpot Apr 3, 2025 • edited Loading

Choose a reason for hiding this comment

brijeshb42 Apr 3, 2025 • edited Loading

Choose a reason for hiding this comment

Janpot Apr 3, 2025

Choose a reason for hiding this comment

michaldudak Apr 3, 2025

Choose a reason for hiding this comment

Janpot Apr 3, 2025 • edited Loading

Choose a reason for hiding this comment

michaldudak Apr 3, 2025

Choose a reason for hiding this comment

Janpot Apr 3, 2025

Choose a reason for hiding this comment

Janpot left a comment • edited Loading

Choose a reason for hiding this comment

brijeshb42 commented Apr 3, 2025 • edited Loading

Janpot commented Apr 3, 2025

brijeshb42 commented Apr 3, 2025

Janpot left a comment • edited Loading

Choose a reason for hiding this comment

michaldudak Apr 7, 2025

Choose a reason for hiding this comment

brijeshb42 Apr 7, 2025

Choose a reason for hiding this comment

Janpot Apr 7, 2025

Choose a reason for hiding this comment

brijeshb42 commented Apr 1, 2025 •

edited

Loading

netlify bot commented Apr 2, 2025 •

edited

Loading

Janpot Apr 3, 2025 •

edited

Loading

brijeshb42 Apr 3, 2025 •

edited

Loading

Janpot Apr 3, 2025 •

edited

Loading

Janpot left a comment •

edited

Loading

brijeshb42 commented Apr 3, 2025 •

edited

Loading

Janpot left a comment •

edited

Loading