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

[license] Remove CLI #5757

Merged
merged 5 commits into from
Aug 26, 2022
Merged

[license] Remove CLI #5757

merged 5 commits into from
Aug 26, 2022

Conversation

flaviendelangle
Copy link
Member

@flaviendelangle flaviendelangle commented Aug 11, 2022

We no longer need the CLI to encode / decode licenses. Related to #4251.

@flaviendelangle flaviendelangle self-assigned this Aug 11, 2022
@flaviendelangle flaviendelangle added the package: x-license Specific to @mui/x-license. label Aug 11, 2022
@mui-bot
Copy link

mui-bot commented Aug 11, 2022

These are the results for the performance tests:

Test case Unit Min Max Median Mean σ
Filter 100k rows ms 335.1 510.4 424.8 437.42 65.226
Sort 100k rows ms 467 895.2 794.7 730.06 168.313
Select 100k rows ms 191.8 276.6 219.6 232.56 30.41
Deselect 100k rows ms 122.1 331 214.6 216.22 67.836

Generated by 🚫 dangerJS against 13545ae

@flaviendelangle flaviendelangle added the bug 🐛 Something doesn't work label Aug 11, 2022
@flaviendelangle flaviendelangle marked this pull request as ready for review August 11, 2022 12:39
@oliviertassinari
Copy link
Member

oliviertassinari commented Aug 11, 2022

I assume it's related to #4251. Looking back at the conversation, one of the constraintes we had changed. We said

The CLI is used by the Studio team and has been fixed in #3965 (and #4055) I even added a CLI recently to decode a license for Matt (#4126)

#4251 (comment) but it doesn't seem to be the case anymore. Our Toolpad support app for MUI X is using the exported JavaScript modules with an ESM import.

Would this work?

  1. We remove the CLI (which could be the only scope of this PR)
  2. We create a new npm package to export generateLicense and decodeLicense which would close [license] Move x-license-pro cli in a new package #4251
  3. We update the Toolpad app to take advantage of it.

@flaviendelangle
Copy link
Member Author

This PR is here to help @mbrookes which can't use the CLI anymore.
So I would not be in favor of doing a new package just for a bug fix.

What's the use case for this? Do you use the CLI through npx in our local repository?
This will break package CLI in environment with NODE_ENV=dev

Indeed, do you have a better check idea ?
Do you use the CLI through npx in our local repository, no but I use node ./bin/license-decode-script.js for instance to test the behavior of the scripts.

@cherniavskii
Copy link
Member

@flaviendelangle

no but I use node ./bin/license-decode-script.js for instance to test the behavior of the scripts.

We can keep only this import:

require('../node/cli/license-cli').licenseDecodeCli();

You can still test it with node ./bin/license-decode-script.js, but you'll have to do this inside the x-license-pro/build directory (or node ./build/bin/license-decode-script.js should work as well).

@oliviertassinari
Copy link
Member

@mbrookes I'm curious why do you need the CLI? Or put it differently, what are you missing from https://master--toolpad.mui.com/_toolpad/app/cl4hla83p01949xoizo5uxf2a/editor/pages/cl4hla87800029xoihl7tf4qz?

@mbrookes
Copy link
Member

mbrookes commented Aug 12, 2022

what are you missing

A date picker. A CLI command is also quicker to complete than a web UI.

@oliviertassinari
Copy link
Member

oliviertassinari commented Aug 12, 2022

A date picker. A CLI command is also quicker to complete than a web UI.

@mbrookes Interesting. I have tried to make it better. I have added default values in Toolpad to match the ones of the CLI. I have also reported how we could use URL parameters mui/mui-toolpad#414 (comment). A quick comparison:

Toolpad: 3.13s

https://master--toolpad.mui.com/app/cl4hla83p01949xoizo5uxf2a/15/pages/cl4hla87800029xoihl7tf4qz

Screen.Recording.2022-08-13.at.00.59.32.mov

vs.

CLI: 4.03s

npx @material-ui/x-license --order 121
Screen.Recording.2022-08-13.at.01.02.57.mov

I think that the main value will come once we delegate the support to less technical agents that won't know how to use the CLI. I got a glimpse of what the pain could be.

@mbrookes
Copy link
Member

mbrookes commented Aug 15, 2022

I've added options for two and three years in Toolpad, and fixed a bug with custom. I think it's good enough for now, even better when the date picker works. We can drop the CLI.

@flaviendelangle flaviendelangle changed the title [license] Fix broken CLI [license] Remove CLI Aug 23, 2022
@flaviendelangle
Copy link
Member Author

CLI removed

@flaviendelangle flaviendelangle merged commit 4b978a9 into mui:master Aug 26, 2022
@flaviendelangle flaviendelangle deleted the cli-fix branch August 26, 2022 08:42
alexfauquette pushed a commit to alexfauquette/mui-x that referenced this pull request Sep 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work package: x-license Specific to @mui/x-license.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants