[charts] Introduce the plugin system #15513

alexfauquette · 2024-11-20T15:47:59Z

This PR brings all the typing and pipeline to support plugins in the ChartProvider.

For now the hooks only replace 2 features:

The svgRef
The chartId

Chart state

The main idea is to have in a provider multiple ref you can ask to interact/render the chart

The instance which contains all the methods to get/update data
The plublicApi that contains the methods we will let people use
The svgRef that already exist
The store introduced in the previous PR that manage state accessible by selectors.

Typing

The plugin types work as follows:

You define the UseChartXxxSignature which defines the state, the interface, ...
You can then use ChartState<[UseChartInteractionSignature, ...]> and it provide a type that merge the instance, and the store of the plugin signatures you listed

mui-bot · 2024-11-20T15:51:39Z

Deploy preview: https://deploy-preview-15513--material-ui-x.netlify.app/

Generated by 🚫 dangerJS against fb4a9fc

codspeed-hq · 2024-11-21T14:13:29Z

CodSpeed Performance Report

Merging #15513 will improve performances by 35.98%

_{Comparing alexfauquette:pluggins (fb4a9fc) with master (7914aa5)}

Summary

⚡ 1 improvements
✅ 5 untouched benchmarks

Benchmarks breakdown

	Benchmark	`master`	`alexfauquette:pluggins`	Change
⚡	`BarChartPro with big data amount`	325.5 ms	239.4 ms	+35.98%

github-actions · 2024-11-22T11:54:13Z

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

packages/x-charts-pro/src/context/ChartDataProviderPro/ChartDataProviderPro.tsx

packages/x-charts/src/ChartsSurface/ChartsSurface.tsx

alexfauquette · 2024-11-22T13:18:41Z

packages/x-charts/src/context/ChartProvider/ChartProvider.tsx

+function ChartProvider(props: ChartProviderProps) {
+  const { children } = props;
+
+  const { contextValue } = useCharts([], {});


Here we will have to propagate the plugins and to props to generate the internal state and add the different methods

what do you mean?

For example the cartesian chart shoudl enable axis processing with something like

<ChartProvider plugins={[ useCartesianAxes ]} >

This plugin would add the x/y axes to the store when doing

const { contextValue } = useCharts([useCartesianAxes], { xAxis, yAxis; ... });

alexfauquette · 2024-11-22T13:28:37Z

packages/x-charts/src/internals/plugins/corePlugins/useChartId/useChartId.ts

+import { UseChartIdSignature } from './useChartId.types';
+import { createChartDefaultId } from './useChartId.utils';
+
+export const useChartId: ChartPlugin<UseChartIdSignature> = ({ params, store }) => {


This is taken from the tree-view.

It allows the user to pass a custom id that override the default one. We don't use that feature we don't have an id prop

probably would be good to add it in the future though

alexfauquette · 2024-11-22T13:36:20Z

packages/x-charts/src/internals/store/useCharts.ts

+
+  if (inputApiRef) {
+    if (inputApiRef.current == null) {
+      // eslint-disable-next-line react-compiler/react-compiler


That's because eslint does not see we are using a ref

Should work once we have facebook/react#29916

flaviendelangle

I don't have enough product context, but the plugin itself seems to be correctly migrated

flaviendelangle · 2024-11-25T10:45:01Z

packages/x-charts/src/internals/store/useCharts.ts

+
+  if (inputApiRef) {
+    if (inputApiRef.current == null) {
+      // eslint-disable-next-line react-compiler/react-compiler


Should work once we have facebook/react#29916

JCQuintas · 2024-11-25T10:56:55Z

packages/x-charts/src/ChartsVoronoiHandler/ChartsVoronoiHandler.tsx

-    if (element === null) {
+    if (svgRef.current === null) {
      return undefined;
    }
+    const element = svgRef.current;


This seems wrong. Technically the current can change from filled to null after the if, but before before we assign it to element. I think eslint used to complain about this 🤔

How could it be? Between the if and the assignment there is no thing, So I don't see how the main JS thread could be interupted

That's why I said "technically" 😅

The ref is a mutable ref, so we can technically change it at any time, if there are async processes that set/remove it, then it could change.

if there are async processes that set/remove it, then it could change.

Not fully sure, but for me JS is mono-thread, which means the current function is executed up to its end, or an await. Only after finishing this execution, the event loop will take other tasks to work on. That's why we sometime use the setTimeout(..., 0) By running a code in async way, we are sure to be executed after the end of React main script

JCQuintas · 2024-11-25T10:57:46Z

packages/x-charts/src/LineChart/LineHighlightPlot.tsx

+import { useStore } from '../internals/store/useStore';
+import { useSelector } from '../internals/store/useSelector';


should we barrel these two together through a internals/store/index.ts?

packages/x-charts/src/internals/plugins/corePlugins/corePlugins.ts

JCQuintas · 2024-11-25T11:03:03Z

packages/x-charts/src/context/ChartProvider/ChartProvider.tsx

+function ChartProvider(props: ChartProviderProps) {
+  const { children } = props;
+
+  const { contextValue } = useCharts([], {});


what do you mean?

JCQuintas · 2024-11-25T11:18:03Z

packages/x-charts/src/internals/plugins/corePlugins/useChartId/useChartId.ts

+import { UseChartIdSignature } from './useChartId.types';
+import { createChartDefaultId } from './useChartId.utils';
+
+export const useChartId: ChartPlugin<UseChartIdSignature> = ({ params, store }) => {


probably would be good to add it in the future though

JCQuintas · 2024-11-25T11:30:33Z

packages/x-charts/src/internals/plugins/corePlugins/useChartId/useChartId.ts

+useChartId.params = {
+  id: true,
+};


where are the these params used? 🤔

In the tree view, they are used to split the props that are forwarded to the DOM and the props that are owned by the plugins and should not be forwarded to the DOM

JCQuintas · 2024-11-25T11:32:55Z

packages/x-charts/src/internals/store/useCharts.ts

+    globalId += 1;
+
+    const initialState = {
+      // TODO remove when the interaction moves to plugin


Is this TODO still relevant? 🤔

Yes, because the interaction pluggin is created but not used.

To move interaction behavior I would need to migrate the useAxisListener, which implies to have access to the drawingArea.

The confusion comes form the fact I started to implement it, but it's not fully operational

JCQuintas · 2024-11-25T11:35:11Z

packages/x-charts/src/internals/store/useStore.ts


-export function useStore(skipError?: boolean): ChartStore {
-  const charts = React.useContext(ChartsContext);
+// This hook should be removed because user and us should not interact with the store directly, but with public/private APIs


When should we remove it?

When the interaction fully move to the plugins, which would allow to remove the hook for axis listener, and voronoi handler which are using the store to update interaction state.

…s.ts Co-authored-by: Jose C Quintas Jr <juniorquintas@gmail.com> Signed-off-by: Alexandre Fauquette <45398769+alexfauquette@users.noreply.github.com>

alexfauquette · 2024-11-25T16:21:03Z

packages/x-charts/src/internals/store/useCharts.ts

+        Object.assign(initialState, plugin.getInitialState({}));
+      }
+    });
+    storeRef.current = new ChartStore(initialState);


@JCQuintas

In this line the state is directly set because we are in the condition storeRef.current == null so we directly compute the initial state.

The discussion we had is about storing data from props into the store.

Since it's a ref we will need to use a useEffect to update the store. For example, the cartesian pluggin could contain to following code to make the xAxis available to selectors

useEffect(() => { store.update(updateXAxis(xAxis)) }, [xAxis])

Which would cause 2 renders. One when user update their prop. A second one due to the store update.

So we trade potential double render with selectors

Small note from the meeting: Unless the chart never uses any selector that contain this data AND the child component (the series? the dot? I don't know what consumes that 😆 ) doesn't have props update or context update when this data changes from the parent.

In which case, we have a first render on the chart and then a second render on the child, but both only render once (except for the mount).

Signed-off-by: Alexandre Fauquette <45398769+alexfauquette@users.noreply.github.com> Co-authored-by: Jose C Quintas Jr <juniorquintas@gmail.com>

move files

681caa5

alexfauquette added the component: charts This is the name of the generic UI component, not the React module! label Nov 20, 2024

WIP

0ffe570

alexfauquette force-pushed the pluggins branch from 6566e4b to 0ffe570 Compare November 21, 2024 12:47

alexfauquette added 3 commits November 21, 2024 14:36

fix

7cc28e9

scripts

c35dce0

fix

0243ebb

alexfauquette added 2 commits November 21, 2024 18:38

Remove SVG ref provider

b77bc5a

fix test

5a6747d

github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Nov 22, 2024

alexfauquette added 2 commits November 22, 2024 13:57

fix

49e49c6

Merge remote-tracking branch 'upstream/master' into pluggins

93cbc3b

github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Nov 22, 2024

alexfauquette changed the title ~~[charts][Draft] Introduce the pluggin system~~ [charts] Introduce the plugin system Nov 22, 2024

alexfauquette added 2 commits November 22, 2024 14:16

move files

5ff63e6

comments

4d5b8a7

alexfauquette commented Nov 22, 2024

View reviewed changes

rsc

d86d191

alexfauquette marked this pull request as ready for review November 22, 2024 13:47

fix

925b4e1

alexfauquette requested review from JCQuintas and flaviendelangle November 22, 2024 15:01

flaviendelangle approved these changes Nov 25, 2024

View reviewed changes

JCQuintas reviewed Nov 25, 2024

View reviewed changes

Update packages/x-charts/src/internals/plugins/corePlugins/corePlugin…

fb4a9fc

…s.ts Co-authored-by: Jose C Quintas Jr <juniorquintas@gmail.com> Signed-off-by: Alexandre Fauquette <45398769+alexfauquette@users.noreply.github.com>

JCQuintas approved these changes Nov 25, 2024

View reviewed changes

alexfauquette commented Nov 25, 2024

View reviewed changes

alexfauquette merged commit db5cf78 into mui:master Nov 26, 2024
19 checks passed

alexfauquette deleted the pluggins branch November 26, 2024 08:57

LukasTy pushed a commit to LukasTy/mui-x that referenced this pull request Dec 19, 2024

[charts] Introduce the plugin system (mui#15513)

75b8d19

Signed-off-by: Alexandre Fauquette <45398769+alexfauquette@users.noreply.github.com> Co-authored-by: Jose C Quintas Jr <juniorquintas@gmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[charts] Introduce the plugin system #15513

[charts] Introduce the plugin system #15513

alexfauquette commented Nov 20, 2024 •

edited

Loading

mui-bot commented Nov 20, 2024 •

edited

Loading

codspeed-hq bot commented Nov 21, 2024 •

edited

Loading

github-actions bot commented Nov 22, 2024

alexfauquette Nov 22, 2024

JCQuintas Nov 25, 2024

alexfauquette Nov 25, 2024

alexfauquette Nov 22, 2024

JCQuintas Nov 25, 2024

alexfauquette Nov 22, 2024

flaviendelangle Nov 25, 2024

flaviendelangle left a comment

flaviendelangle Nov 25, 2024

JCQuintas Nov 25, 2024

alexfauquette Nov 25, 2024

JCQuintas Nov 25, 2024

alexfauquette Nov 25, 2024

JCQuintas Nov 25, 2024

JCQuintas Nov 25, 2024

JCQuintas Nov 25, 2024

JCQuintas Nov 25, 2024

flaviendelangle Nov 25, 2024

JCQuintas Nov 25, 2024

alexfauquette Nov 25, 2024 •

edited

Loading

JCQuintas Nov 25, 2024

JCQuintas Nov 25, 2024

alexfauquette Nov 25, 2024

alexfauquette Nov 25, 2024

flaviendelangle Nov 25, 2024

		import { useStore } from '../internals/store/useStore';
		import { useSelector } from '../internals/store/useSelector';

[charts] Introduce the plugin system #15513

[charts] Introduce the plugin system #15513

Conversation

alexfauquette commented Nov 20, 2024 • edited Loading

Chart state

Typing

mui-bot commented Nov 20, 2024 • edited Loading

codspeed-hq bot commented Nov 21, 2024 • edited Loading

Merging #15513 will improve performances by 35.98%

Summary

Benchmarks breakdown

github-actions bot commented Nov 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flaviendelangle left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexfauquette Nov 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexfauquette commented Nov 20, 2024 •

edited

Loading

mui-bot commented Nov 20, 2024 •

edited

Loading

codspeed-hq bot commented Nov 21, 2024 •

edited

Loading

alexfauquette Nov 25, 2024 •

edited

Loading