-
Notifications
You must be signed in to change notification settings - Fork 373
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
Migrate @mui/x-date-pickers to v6 #2139
Migrate @mui/x-date-pickers to v6 #2139
Conversation
✅ Deploy Preview for jsonforms-examples ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
I have noticed, that the Netlify prerview for materia renderers does not work. After checking the browser console logs I encountered the following error:
It seems that the Netlify deployment uses an older version of @mui than specified in the dependencies. |
@sebastianfrey That is strange as the example apps are bundled to be self-contained in their corresponding packages. Can you also reproduce this issue locally? |
@lucas-koehler Yes I can, when I run locally |
@lucas-koehler After a little bit more investigation it turns out, that the problem originates from the required @mui/material upgrade. In detail the reason for the problem is b. how @mui/utils is published (Transpiled ES-Module) It seems that rollup/plugins#948 is related and that rollup/plugins#1165 might fix it. Unfortunately I have not the time right now to dig here deeper, maybe some of you guys might take a look? UPDATE 2023-05-25: Yesterday I was browsing through some GitHub-issues on the topic and read up a bit on how Rollup plugins do work. Actually I found a solution to the problem, but it is a bit hacky. As I have already mentioned, the cause of the problem is on the one hand the way @mui/utils is published and on the other hand how the Rollup CommonJS plugin handles these imports. To come around the problem, I have created the following Rollup plugin: diff --git a/packages/material-renderers/rollup.example.config.js b/packages/material-renderers/rollup.example.config.js
index 1cc764d0..e46b6711 100644
--- a/packages/material-renderers/rollup.example.config.js
+++ b/packages/material-renderers/rollup.example.config.js
@@ -6,6 +6,36 @@ import copy from 'rollup-plugin-copy';
import css from 'rollup-plugin-import-css';
import typescript from 'rollup-plugin-typescript2';
+function cjsCompatPlugin() {
+ return {
+ name: 'cjs-compat-plugin',
+ transform(code, id) {
+ // ignore all packages which are not @mui/utils
+ if (
+ !/@mui\/utils.*.js$/.test(id) ||
+ id.includes('@mui/utils/node_modules')
+ ) {
+ return code;
+ }
+
+ // try to extract the commonjs namespace variable
+ const moduleName = code.match(
+ /(?<module>[a-zA-Z0-9_$]*).default = _default;/
+ )?.groups?.module;
+
+ if (!moduleName || !code.includes(`return ${moduleName};`)) {
+ return code;
+ }
+
+ // return default export instead of namespace
+ return code.replace(
+ `return ${moduleName}`,
+ `return ${moduleName}.default`
+ );
+ },
+ };
+}
+
/**
* @type {import('rollup').RollupOptions}
*/
@@ -34,6 +64,7 @@ const config = {
},
},
}),
+ cjsCompatPlugin(),
copy({
targets: [
{ It basically mutates the generated CommonJS code by returning the default export instead of the namespace. If you're happy with that solution, I will update the PR. |
Hi @sebastianfrey , |
Hi @lucas-koehler, Here you go. Custom plugin and brief comment are pushed as requested. =) |
Hi @sebastianfrey , thanks for the fixes and the effort :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks again for the contribution ❤️
The changes already look pretty good to me.
I tried locally, and I didn't need the introduced PickersActionBarParams
. Could you check again if we really need this, please?
Besides that, I tested the new inputs in the deployed example and it looks good to me. With the new fields mechanism, many invalid inputs are no longer possible and in case the value is deleted or incomplete, "Invalid Date"
is written to the data. This seems fine to me. @sdirix Do you agree?
UI Schema based controls do not show a validation message for invalid data. However, this is already the case without this PR. So this is fine for me.
packages/material-renderers/src/controls/MaterialDateControl.tsx
Outdated
Show resolved
Hide resolved
9c1d37a
to
931cd66
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the updates, LGTM now :)
@lucas-koehler Just out of curiosity: What's the timeline for v3.1.0? |
@sebastianfrey There is no fixed timeline for the 3.1.0 release, yet. Most likely there will be at least one more consumable alpha or beta version beforehand. |
The change is released as part of the |
@sdirix @lucas-koehler Thanks for your effort and fast response. I appreciate this. |
Resolves #2137.
There are some things to consider:
<ResetableTextField />
component and the custom text input value handling. Both were introduced in Pass through original values for invalid dates #1949. Unfortunately, as I understand the MUI release blog article correctly, there is no way at the moment to disable the masked input behavior. That means the behavior introduced in Pass through original values for invalid dates #1949 cannot be replicated with v6.