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

[pickers] Add "use client" directive to every public component and hook #14562

Merged
merged 7 commits into from
Sep 11, 2024

Conversation

flaviendelangle
Copy link
Member

@flaviendelangle flaviendelangle commented Sep 10, 2024

Part of #9833

I copy pasted the script from the core.
@alexfauquette do you think it's worth splitting the logic and the projects on the core repo so that we could re-use the logic?

@flaviendelangle flaviendelangle self-assigned this Sep 10, 2024
@flaviendelangle flaviendelangle added the component: pickers This is the name of the generic UI component, not the React module! label Sep 10, 2024
@flaviendelangle flaviendelangle changed the title [pickers] Add use client directive [pickers] Add "use client" directive Sep 10, 2024
@flaviendelangle flaviendelangle changed the title [pickers] Add "use client" directive [pickers] Add "use client" directive to every public component and hook Sep 10, 2024
@mui-bot
Copy link

mui-bot commented Sep 10, 2024

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

Generated by 🚫 dangerJS against b66fdab

@flaviendelangle flaviendelangle marked this pull request as ready for review September 10, 2024 14:11
@alexfauquette
Copy link
Member

I copy pasted the script from the core.

I don't have strong preferences. In a ideal world, I would publish this package from the core to allow running commands such that npx rsc-builder@latest ./packages/x-date-pickers/

@flaviendelangle
Copy link
Member Author

But you would need to store the ignorePaths of each project (the pickers don't have any for now), not sure the CLI is doable 😬

@alexfauquette
Copy link
Member

the pickers don't have any for now

That what led me to this direction. The project config are so simple that I did not understand why copy pasting this

I tried to extract the login in a resolveProject in mui/material-ui#43693 but it breaks the script because of some relative path (it's looking for /mui-x/packages/mui-base/src)

If we plan to use this script in some pipeline, yes it would be nice to maintain this code in a single place. But if it's a one shot script, I don't think it is worth the effort

@flaviendelangle
Copy link
Member Author

If we plan to use this script in some pipeline, yes it would be nice to maintain this code in a single place. But if it's a one shot script, I don't think it is worth the effort

I'm curious to know why the core created a standalone package for this instead of a simple script file (cc @brijeshb42 @siriwatknp).

@brijeshb42
Copy link

@mj12albert could answer that

@JCQuintas
Copy link
Member

@alexfauquette I would argue that when we implement this, it should also have a check on CI to ensure we are using it 😅

@flaviendelangle
Copy link
Member Author

@alexfauquette I would argue that when we implement this, it should also have a check on CI to ensure we are using it 😅

I did not see such a check on the core, but I do agree that it would a nice addition

packages/x-date-pickers/src/DatePicker/shared.tsx Outdated Show resolved Hide resolved
packages/x-date-pickers/src/TimeClock/ClockNumber.tsx Outdated Show resolved Hide resolved
@@ -29,6 +29,7 @@
"prettier:all": "prettier --write . --ignore-path .eslintignore",
"prettier:check": "prettier --check . --ignore-path .eslintignore",
"proptypes": "cross-env BABEL_ENV=development babel-node -i \"/node_modules/(?!@mui)/\" -x .ts,.tsx,.js ./docs/scripts/generateProptypes.ts",
"rsc:build": "tsx ./packages/rsc-builder/buildRsc.ts",
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be part of CI? 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

It is not in the core, but it would be a nice addition
I'm trying to add it 👍

Choose a reason for hiding this comment

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

It might be a tricky process to do in CI since you'll then want the code to be committed back.
Ideally, it should be part of the build (or did you mean build) ?

Copy link
Member Author

Choose a reason for hiding this comment

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

The CI reminds you to run the script locally anytime you create a new public component (if you did not add "use client" manually).
Similar to what we have for pnpm l10n, it does not commit the localization files but it makes sure we don't forget to do it.

Copy link
Member

Choose a reason for hiding this comment

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

@brijeshb42
It is either part of the build, and we don't commit anything, or it can be a check, where if there are changes after the script is run, the CI fails.

@LukasTy LukasTy added the core Infrastructure work going on behind the scenes label Sep 10, 2024
Copy link
Member

@LukasTy LukasTy left a comment

Choose a reason for hiding this comment

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

LGTM. 👌
Thanks for taking care of it and fine-tuning the solution. 👍

@flaviendelangle flaviendelangle merged commit 3d227ff into mui:master Sep 11, 2024
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: pickers This is the name of the generic UI component, not the React module! core Infrastructure work going on behind the scenes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants