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

[core] Migrate @mui/x-license-pro to the new bundling strategy #3738

Merged
merged 36 commits into from
Feb 14, 2022

Conversation

flaviendelangle
Copy link
Member

@flaviendelangle flaviendelangle commented Jan 25, 2022

Codesandbox playground

Part of #18
Part of #3608

What's next ?

  1. Migrate @mui/x-data-grid-generator
  2. Migrate @mui/x-data-grid-pro after [DataGridPro] Make @mui/x-data-grid-pro import shared code from @mui/x-data-grid #3688
  3. Migrate @mui/x-data-grid after the total removal of _modules_

@flaviendelangle flaviendelangle added on hold There is a blocker, we need to wait core Infrastructure work going on behind the scenes labels Jan 25, 2022
@flaviendelangle flaviendelangle self-assigned this Jan 25, 2022
@flaviendelangle flaviendelangle changed the title [core] Migrate @mui/x-license-pro to the new bundling strategy [core] Migrate @mui/x-license-pro to the new bundling strategy Jan 25, 2022
@mui-bot
Copy link

mui-bot commented Jan 25, 2022

These are the results for the performance tests:

Test case Unit Min Max Median Mean σ
Filter 100k rows ms 225.8 468.6 391 364.84 80.299
Sort 100k rows ms 399.2 820 725.1 685.8 147.533
Select 100k rows ms 183.3 268.6 210.8 216.48 28.152
Deselect 100k rows ms 106.7 300.8 178.3 192.7 67.412

Generated by 🚫 dangerJS against bacb7d8

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 25, 2022
@github-actions
Copy link

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Feb 1, 2022
plugins: [
[
'transform-react-remove-prop-types',
{
ignoreFilenames: ['DataGridPro.tsx'],
},
],
[
'search-and-replace',
Copy link
Member Author

Choose a reason for hiding this comment

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

I moved this replace to babel because once we remove rollup for @mui/x-data-grid-pro we won't be able to use to current plugin.

"directory": "packages/x-license-pro"
},
"types": "build/index.d.ts",
"license": "SEE LICENSE IN LICENSE.txt",
Copy link
Member Author

Choose a reason for hiding this comment

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

We now need a valid license field for bundling

@@ -57,7 +57,8 @@
"build:codesandbox": "yarn release:build",
"release:changelog": "node scripts/releaseChangelog",
"release:version": "lerna version --exact --no-changelog --no-push --no-git-tag-version",
"release:build": "cd packages/x-license-pro && yarn build && cd ../grid/x-data-grid-pro && yarn build && cd ../x-data-grid && yarn build && cd ../x-data-grid-generator && yarn build",
"release:build": "yarn release:build:next && cd packages/grid/x-data-grid-pro && yarn build && cd ../x-data-grid && yarn build && cd ../x-data-grid-generator && yarn build",
"release:build:next": "lerna run --parallel --scope \"@mui/x-license-pro\" 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.

I'm following mui/material-ui#30201, mui/material-ui#30200 because we will probably have the same issue.
But for now it's not a problem since I build the only package with the core bundling strategy before the others.

NotFound = 'NotFound',
Invalid = 'Invalid',
Expired = 'Expired',
Valid = 'Valid',
}

export { LicenseStatus };
Copy link
Member Author

Choose a reason for hiding this comment

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

I have a weird compilation error on Codesandbox otherwise when using our ESM bundle as the input.
It gets the following output:

exports.LicenseStatus = void  0;
Object.defineProperty(exports, "__esModule", {
  value: true
});
let LicenseStatus;
exports.LicenseStatus = LicenseStatus;
(function (LicenseStatus) {
  LicenseStatus["NotFound"] = "NotFound";
  LicenseStatus["Invalid"] = "Invalid";
  LicenseStatus["Expired"] = "Expired";
  LicenseStatus["Valid"] = "Valid";
})(LicenseStatus || (LicenseStatus = {}));

And exports.LicenseStatus remains undefined (you can test in the console).

The changes in the code allow to init the enum before exporting it, which solves the problem.

Copy link
Member

Choose a reason for hiding this comment

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

@michaldudak I think you've experienced similar issue with Codesandbox recently. What was your solution to that?

Copy link
Member Author

@flaviendelangle flaviendelangle Feb 7, 2022

Choose a reason for hiding this comment

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

Replace

export enum MyEnum { ... }

With

enum MyEnum {...}

export { MyEnum }

That way you have something like

exports.LicenseStatus = void  0;
Object.defineProperty(exports, "__esModule", {
  value: true
});
let LicenseStatus;
-exports.LicenseStatus = LicenseStatus;
(function (LicenseStatus) {
  LicenseStatus["NotFound"] = "NotFound";
  LicenseStatus["Invalid"] = "Invalid";
  LicenseStatus["Expired"] = "Expired";
  LicenseStatus["Valid"] = "Valid";
})(LicenseStatus || (LicenseStatus = {}));
+exports.LicenseStatus = LicenseStatus;

Copy link
Member

Choose a reason for hiding this comment

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

It looks like an error in the Codesandbox' bundler. I created an issue in their repo: codesandbox/codesandbox-client#6435

@flaviendelangle nice work finding a workaround!

Copy link
Member Author

Choose a reason for hiding this comment

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

Always a pleasure to dive into compiled code with 5mn between each test due to the build duration 😆

@@ -1,24 +1,5 @@
const path = require('path');
Copy link
Member Author

Choose a reason for hiding this comment

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

I synced this file with the core one, minus some things (we don't have there error management for instance).
I will do some tests for the packages still on the old bundling. But I would advise against releasing a state where some package use the core bundling strategy and others don't.

@flaviendelangle flaviendelangle marked this pull request as ready for review February 3, 2022 16:03
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Feb 6, 2022
@github-actions
Copy link

github-actions bot commented Feb 6, 2022

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Feb 7, 2022
@siriwatknp
Copy link
Member

siriwatknp commented Feb 8, 2022

👋 The migration PR has been merged.

Please follow these steps to make sure that the contents are all updated. (Sorry for the inconvenience)

  1. pull latest master from upstream to your branch
  2. if your PR has changes on the *.md or demo files, you are likely to encounter conflict because all of them have been moved to the new folder.
    2.1 revert the change on those markdown files you did
    2.2 pull latest master from upstream (you should not see conflict)
    2.3 make the changes again at docs/data/data-grid/*
  3. run yarn docs:api
    • you might see the changes in docs/pages/x/api/data-grid/* if your PR touches some of the components
    • it is okay if there is no changes

If you are struggle with the steps above, feel free to tag @siriwatknp

Copy link
Member

@m4theushw m4theushw left a comment

Choose a reason for hiding this comment

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

I don't have too much to say here but everything seems to still work as expected. We will need to pay more attention when migrating @mui/x-data-grid.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Infrastructure work going on behind the scenes on hold There is a blocker, we need to wait
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants