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] Add TS definition to the exported elements #9885

Merged
merged 2 commits into from
Aug 2, 2023

Conversation

alexfauquette
Copy link
Member

I created a basic nextjs with charts to reproduce this issue comment. It failed to build because of missing TS definition.

https://github.com/alexfauquette/charts-next-test

The modification in this PR made directly in the node_module allowed to build

@alexfauquette alexfauquette added the component: charts This is the name of the generic UI component, not the React module! label Aug 2, 2023
@mui-bot
Copy link

mui-bot commented Aug 2, 2023

Netlify deploy preview

Netlify deploy preview: https://deploy-preview-9885--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 312.3 496.1 363 377.16 68.229
Sort 100k rows ms 376.1 798.2 685.2 653.72 153.645
Select 100k rows ms 127.3 276 215.8 201.18 49.954
Deselect 100k rows ms 99.7 291.8 205 175 67.523

Generated by 🚫 dangerJS against e99dc1e

@alexfauquette alexfauquette requested a review from Janpot August 2, 2023 08:39
@Janpot
Copy link
Member

Janpot commented Aug 2, 2023

Could work, I'm used to seeing it this way:

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

Never seen that fail, though Typescript seems to discourage the reuse of declaration files for both ESM and CommonJS.

@alexfauquette
Copy link
Member Author

To get it works with nested imports such as @muo/x-charts/LineChart I needed the following diff

  "exports": {
    ".": {
      "types": "./index.d.ts",
      "require": "./index.js",
      "import": "./esm/index.js"
    },
    "./*": {
-      "types": "./*.d.ts",
+      "types": "./*/index.d.ts",
      "require": "./*",
      "import": "./esm/*"
    }
  }

If others found this PR the reason is node subpath patterns:

Even if it uses *, the pattern matching does not work like .gitignore config files. The * in ./* can be any string including /.

So for @mui/x-charts/LineChart the ./* will simply do * = LineChart and then look into build/LineChart.d.ts which does not exist. With the modification it will look in build/LineChart/index.d.ts

@alexfauquette alexfauquette merged commit c1094d3 into mui:master Aug 2, 2023
@alexfauquette alexfauquette deleted the fix-ts-import branch August 2, 2023 09:53
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!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants