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

[charts] Use ESM with imports #9645

Merged
merged 3 commits into from
Jul 20, 2023
Merged

Conversation

alexfauquette
Copy link
Member

@alexfauquette alexfauquette commented Jul 11, 2023

Iterate on #9556

@mui-bot
Copy link

mui-bot commented Jul 11, 2023

Netlify deploy preview

Netlify deploy preview: https://deploy-preview-9645--material-ui-x.netlify.app/

Updated pages

No updates.

These are the results for the performance tests:

Test case Unit Min Max Median Mean σ
Filter 100k rows ms 318.5 467.8 401.9 387.58 55.575
Sort 100k rows ms 514.8 948.9 819.5 783.24 153.329
Select 100k rows ms 208.5 261.1 241.5 238.7 18.882
Deselect 100k rows ms 151.7 240.9 204.6 194.14 35.568

Generated by 🚫 dangerJS against e41dcd2

@alexfauquette
Copy link
Member Author

@Janpot I think I found an intermediate solution by using the following config in the package.json

If I'm correct devs using the import will get ESM so no problem for them. Those using the require will get commonJS but in that case, they need a way to get the d3 in common JS

  "exports": {
    ".": {
      "require": "./index.js",
      "import": "./esm/index.js"
    },
    "./*": {
      "require": "./*",
      "import": "./esm/*"
    }
  },

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

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

@Janpot
Copy link
Member

Janpot commented Jul 18, 2023

I believe this would implement proposed strategy 2 from mui/material-ui#37335.

I find it difficult to take a stance on which strategy to follow as any of them will have an impact on some of our users. My personal preference though would be that we follow the same strategy throughout all MUI packages that need to interoperate as to maximize the probability of them working together successfully in our users wide range of standards compliant and less-compliant setups.

As an example, the icons package deviates from our regular strategy and I keep running into problems with bundlers misinterpreting it.

I'm not against this PR, I think I'd slightly prefer more that we just not externalize the d3 packages and bundle them in our output. Did that strategy work?

@alexfauquette
Copy link
Member Author

I did not succeed in bundling the d3- libraries in the charts.

From the toolpad codebase I saw you used the tsup noExternal options to do that. But did not find similar properties in tsconfig

And I'm confused because the webpack externals is pointing a given set of libraries. So by default others would be considered as internals
https://github.com/mui/material-ui/blob/2c87a32e886e96040e6f5112895ce63187735b55/scripts/sizeSnapshot/webpack.config.js#L178

@Janpot
Copy link
Member

Janpot commented Jul 19, 2023

Oh right, I see, we are not bundling at all, we're just transforming our input files. I'm sorry, that makes my suggestion a bit harder than I expected. I believe we have roughly the same options as described by https://formidable.com/blog/2022/victory-esm/. It's either becoming ESM-only or vendoring in d3. (unless not using d3 is also an option 🙂 )

(cc @mui/code-infra thoughts?)

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Jul 20, 2023
@alexfauquette alexfauquette marked this pull request as ready for review July 20, 2023 08:59
@alexfauquette alexfauquette merged commit a096255 into mui:master Jul 20, 2023
@alexfauquette alexfauquette deleted the export-fix branch July 20, 2023 09:13
@michaldudak
Copy link
Member

As Charts are not stable yet, I believe we may use this package to verify the solution regarding ESM. If it proves to be painless for the community, we'll be more certain it won't cause major problems when eventually other packages follow the same pattern.

@zannager zannager added the component: charts This is the name of the generic UI component, not the React module! label Jul 20, 2023
MBilalShafi pushed a commit that referenced this pull request Aug 2, 2023
@oliviertassinari
Copy link
Member

I have tried to prefix all the issues we have about publishing JavaScript to the best possible format with [ESM]: https://github.com/search?q=org%3Amui+%22%5BESM%5D%22++&type=issues&state=open.

Good luck https://blog.isquaredsoftware.com/2023/08/esm-modernization-lessons/ 😅

@oliviertassinari oliviertassinari added the core Infrastructure work going on behind the scenes label Aug 12, 2023
@oliviertassinari oliviertassinari changed the title [charts] use ESM with imports [charts] Use ESM with imports Jan 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: charts 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