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

Migrate @mui/x-date-pickers to v6 #2139

Conversation

sebastianfrey
Copy link
Contributor

Resolves #2137.

There are some things to consider:

  1. Because @mui/date-pickers v6 relies on a newer version of @mui/base I had to upgrade @mui/material to a more recent version (v5.13.0).
  2. There are no public types for the functional ActionBar API, so I had to introduce a custom interface to satisfy TS.
  3. The input handling of the date picker components was reworked from the ground up in v6. For more details see here. For that reason I had to drop the custom <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.

@CLAassistant
Copy link

CLAassistant commented May 23, 2023

CLA assistant check
All committers have signed the CLA.

@netlify
Copy link

netlify bot commented May 23, 2023

Deploy Preview for jsonforms-examples ready!

Name Link
🔨 Latest commit 931cd66
🔍 Latest deploy log https://app.netlify.com/sites/jsonforms-examples/deploys/6478611016901a000807189a
😎 Deploy Preview https://deploy-preview-2139--jsonforms-examples.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

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

@sebastianfrey
Copy link
Contributor Author

sebastianfrey commented May 23, 2023

I have noticed, that the Netlify prerview for materia renderers does not work. After checking the browser console logs I encountered the following error:

react-dom.production.min.js:216 TypeError: useEnhancedEffectExports is not a function
    at usePickerViews (usePickerViews.js:100:3)
    at usePicker (usePicker.js:28:31)
    at useDesktopPicker (useDesktopPicker.js:59:7)
    at DesktopDatePicker (DesktopDatePicker.js:49:7)
    at Ch (react-dom.production.min.js:157:137)
    at gi (react-dom.production.min.js:176:260)
    at ck (react-dom.production.min.js:271:400)
    at bk (react-dom.production.min.js:250:347)
    at ak (react-dom.production.min.js:250:278)
    at Tj (react-dom.production.min.js:250:138)

It seems that the Netlify deployment uses an older version of @mui than specified in the dependencies.

@lucas-koehler
Copy link
Contributor

I have noticed, that the Netlify prerview for materia renderers does not work. After checking the browser console logs I encountered the following error:

react-dom.production.min.js:216 TypeError: useEnhancedEffectExports is not a function
    at usePickerViews (usePickerViews.js:100:3)

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?

@coveralls
Copy link

coveralls commented May 24, 2023

Coverage Status

Coverage: 84.254%. Remained the same when pulling 931cd66 on sebastianfrey:2137-migrate-mui-date-pickers-to-v6 into b11f04c on eclipsesource:master.

@sebastianfrey
Copy link
Contributor Author

@lucas-koehler Yes I can, when I run locally npm run build:examples-app to build the example apps, the issue is reproducible. Running the example app in dev mode with cd packages/material-renderers && npm run dev everything works fine. Maybe it has something to do with how rollup handles dependencies.

@sebastianfrey
Copy link
Contributor Author

sebastianfrey commented May 24, 2023

@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)
c. how @mui/utils is leveraged in @mui/x-date-pickers
d. how Rollup handles CommonJS default exports of transpile ES-Modules

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.

@lucas-koehler
Copy link
Contributor

Hi @sebastianfrey ,
thank you for your detailed investigation :) . As the examples build is solely used for dev purposes to deploy the examples page, using this "hacky" custom plugin is fine with me. When you update the PR, please just add a brief comment above the plugin definition why this is necessary so its purpose is immediately clear when looking at this config in the future.

@sebastianfrey
Copy link
Contributor Author

Hi @lucas-koehler,

Here you go. Custom plugin and brief comment are pushed as requested. =)

@lucas-koehler
Copy link
Contributor

Hi @sebastianfrey , thanks for the fixes and the effort :)
I'll have a closer look at the changes in June as I'm low on time right now.

Copy link
Contributor

@lucas-koehler lucas-koehler left a 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.

image

@sebastianfrey sebastianfrey force-pushed the 2137-migrate-mui-date-pickers-to-v6 branch from 9c1d37a to 931cd66 Compare June 1, 2023 09:12
Copy link
Contributor

@lucas-koehler lucas-koehler left a 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 lucas-koehler merged commit 5d5d3b5 into eclipsesource:master Jun 5, 2023
9 checks passed
@sebastianfrey
Copy link
Contributor Author

@lucas-koehler Just out of curiosity: What's the timeline for v3.1.0?

@lucas-koehler lucas-koehler modified the milestone: 3.1 Jun 5, 2023
@lucas-koehler
Copy link
Contributor

@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.

@sdirix
Copy link
Member

sdirix commented Jun 5, 2023

The change is released as part of the 3.1.0-beta.0 pre-release (@next) which is available now

@sebastianfrey
Copy link
Contributor Author

@sdirix @lucas-koehler Thanks for your effort and fast response. I appreciate this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

JSONForms >= 3.0.0 is incompatible with @mui/x-date-pickers >= 6.0.0
5 participants