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] Make useReactVersion script reusable in other repos #42828

Merged
merged 2 commits into from
Jul 18, 2024

Conversation

cherniavskii
Copy link
Member

@cherniavskii cherniavskii commented Jul 2, 2024

For MUI X, I duplicated the useReactVersion.mjs script in mui/mui-x#13360, but eventually, I want to use the script from the monorepo.
I've extracted the CLI to a separate file, so now other repos ca import the setReactVersion function.

@cherniavskii cherniavskii added the scope: code-infra Specific to the core-infra product label Jul 2, 2024
@mui-bot
Copy link

mui-bot commented Jul 2, 2024

Netlify deploy preview

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

Bundle size report

No bundle size changes (Toolpad)
No bundle size changes

Generated by 🚫 dangerJS against 73ec511

@cherniavskii cherniavskii marked this pull request as ready for review July 2, 2024 19:45
@cherniavskii cherniavskii requested a review from a team July 2, 2024 19:45
@Janpot
Copy link
Member

Janpot commented Jul 4, 2024

Not going to block this PR, but if it's just to pass a workspace root path I'd consider just sharing the cli like we do with the changelog script. Less code to maintain and less implicit API shared between repos.

@cherniavskii
Copy link
Member Author

@Janpot Hmm, do you mean something like this? node ./useReactVersion.mjs /workspace/root/path?
I don't have a strong preference, I can do that 👍🏻

@Janpot
Copy link
Member

Janpot commented Jul 6, 2024

Yes, it's just a suggestion, I have a preference but it's not a strong one neither 😄

@cherniavskii
Copy link
Member Author

@Janpot I looked into this, and it seems that I have to use the getWorkspaceRoot function anyway to always get the correct workspace path regardless of the directory from which the script was called. Alternatively, I can use process.cwd(), but that would lead to different results depending on the CWD (e.g. node ./scripts/useReactVersion.mjs and cd scripts && node useReactVersion.mjs would behave differently).
Given the above, I would prefer keeping the current approach with function export.
Does this make sense to you? Or am I missing something?

@Janpot
Copy link
Member

Janpot commented Jul 15, 2024

@Janpot I looked into this, and it seems that I have to use the getWorkspaceRoot function anyway to always get the correct workspace path regardless of the directory from which the script was called. Alternatively, I can use process.cwd(), but that would lead to different results depending on the CWD (e.g. node ./scripts/useReactVersion.mjs and cd scripts && node useReactVersion.mjs would behave differently).

When the script is run as a package.json script then process.cwd() will always be the folder that contains the package.json, even when run from a subfolder. My hope is for our internal tooling to mimic tools like eslint and prettier and distribute them as a CLI. In fact, In time I'd like to add a bin script to our repo so that dependent projects can just do something like "use-react-version": "mui-infra use-react-version" without any other setup required.

@cherniavskii
Copy link
Member Author

When the script is run as a package.json script then process.cwd() will always be the folder that contains the package.json

Right, I didn't think about this!
Added a use-react-version script and used process.cwd() instead of getWorkspaceRoot() 👍🏻

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.

perfect

@cherniavskii cherniavskii changed the title [code-infra] Make useReactVersion script reusable in other repos [code-infra] Make useReactVersion script reusable in other repos Jul 18, 2024
@cherniavskii cherniavskii merged commit 0d9333e into mui:next Jul 18, 2024
19 checks passed
@cherniavskii cherniavskii deleted the reusable-useReactVersion branch July 18, 2024 09:39
joserodolfofreitas pushed a commit to joserodolfofreitas/material-ui that referenced this pull request Jul 29, 2024
@michaldudak
Copy link
Member

As a next step, we could move it to @mui/internal-scripts so it can be referenced from the npm package in other repos.

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.

5 participants