LG-4662: Fix bundle size + other charts improvements #2611
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
🦋 Changeset detectedLatest commit: 68595ff The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
tsck
commented
Dec 18, 2024
charts/core/src/Echart/useEchart.ts
Outdated
Comment on lines
96
to
103
| const withInstanceCheck = <T extends (...args: Array<any>) => void>( | ||
| fn: T, | ||
| ) => { | ||
| return (...args: Parameters<T>) => { | ||
| if (!echartsInstance) { | ||
| console.error('Echart instance not initialized'); | ||
| return; | ||
| } | ||
| fn(...args); | ||
| }; | ||
| }; |
There was a problem hiding this comment.
I went back and forth on whether this should actually wait for an instance. In theory, we could store a promise in a ref, and make all of the methods asynchronous. I ultimately decided not to and instead essentially do nothing if the instance doesn't exist yet. But would be curious to hear other thoughts.
There was a problem hiding this comment.
I'm a little confused why this is necessary. Is it just for the error logging?
Unless I'm missing something, the optional chain check (e.g. L114) should catch if the echartsInstance singleton is undefined/null.
Edit: I guess it's useful to fully abort if the instance isn't defined
There was a problem hiding this comment.
If this were to wait for the instance, that would mean all the instance methods (like addSeries) would all be async?
There was a problem hiding this comment.
Edit: I guess it's useful to fully abort if the instance isn't defined
That's correct. This is used as a higher order function to wrap any function that relies on the existence of an instance. This is an extraction of duplicate logic that was being used inline in each of those methods, attempting to handle the case where it doesn't exist yet gracefully.
If this were to wait for the instance, that would mean all the instance methods (like addSeries) would all be async?
That's correct. It would basically wrap the given function in a promise, converting it to an async function that's capable of being awaited, so instead of just (almost) silently ignoring this case, the execution wouldn't occur until the instance did exist.
There was a problem hiding this comment.
Fwiw, I don't think I like this. I think the interface of just exposing some way to determine readiness is fine.
|
Size Change: +6.67 kB (+0.48%) Total Size: 1.4 MB
ℹ️ View Unchanged
|
| const { theme } = useDarkMode(); | ||
|
|
||
| useEffect(() => { | ||
| if (!chart.ready) return; |
There was a problem hiding this comment.
I might be misunderstanding how echarts works, but why can't we check if chart is defined?
There was a problem hiding this comment.
chart in this case is an instance of ChartInstance. In theory, that's actually always defined even when the echart hasn't been initialized. We could check if chart._echartsInstance is defined, but I just added chart.ready as a helper boolean to avoid having to interact directly with the instance outside of useEchart. Does that make sense?
| const [echartsInstance, setEchartsInstance] = useState<any>(null); // has to be any since no types exist until import | ||
| const [error, setError] = useState<EChartsInstance['error']>(null); | ||
| const [ready, setReady] = useState<EChartsInstance['ready']>(false); | ||
| const [options, setOptions] = useState<EChartsInstance['options']>( |
There was a problem hiding this comment.
remind me, why do we need to keep track of options in state if we can access them via echartsInstance.options?
There was a problem hiding this comment.
If we want it more easily accessible from this hook's return object, could we just have
const options = useMemo(() => echartsInstance.options, [...]) // or whatever the correct property is?There was a problem hiding this comment.
I think at one point early on I was tying into changes in order to trigger updates. Since this is no longer the case, I think this is a really good point. Will look into removing this.
There was a problem hiding this comment.
Okay I dug into this more and remember why we were holding a copy of the options. This is what made it possible to batch updates. Since we debounce calls to echartsInstance.setOption() via setEchartOptions, when additional updates occur in short order, they don't have an accurate copy of the current options since echartsInstance.getOption() is not necessarily up to date yet.
I know there was some initial disagreement regarding whether this batching was necessary. I tried to run some tests rendering a chart with 100 lines. With batching, the chart appeared after about 4000ms. Without batching, it didn't appear until after the profiler stopped recording which seemed to be around 6000ms. I'm not 100% sure what caused this but I am under the impression that each additional update is supposed to cause a repaint of the canvas, so it would be 1 repaint vs 100+ repaints. Though, I have no idea if that's what caused this, it could be something totally unrelated, at least withe current implementation it caused an inherent slowdown.
Happy to discuss further of course if there are other ideas, but just going to leave as is for now.
Comment on lines
+10
to
+18
| const mockEchartsInstance = { | ||
| setOption: jest.fn(), | ||
| dispose: jest.fn(), | ||
| resize: jest.fn(), | ||
| on: jest.fn(), | ||
| off: jest.fn(), | ||
| dispatchAction: jest.fn(), | ||
| group: null, | ||
| }; |
| updateChartOptions: (newOptions: Partial<ChartOptions>) => void; | ||
| addChartSeries: (series: SeriesOption) => void; | ||
| removeChartSeries: (name: string) => void; | ||
| chart: ChartInstance; |
There was a problem hiding this comment.
could this be undefined?
There was a problem hiding this comment.
ChartInstance, the object with the instance methods, I don't believe should be, but props like ChartInstance._echartsInstance could be null.
tsck
force-pushed
the
terrence/fix-bundle-size
branch
from
44436b4 to
eea4d45
Compare
tsck
force-pushed
the
terrence/fix-bundle-size
branch
from
1bf408e to
68595ff
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
✍️ Proposed changes
offsetWidthbug.useEcharthook.ChartInstancein full via theChartProviderinstead of individual methods.🎟 Jira tickets:
offsetWidtherroruseChart✅ Checklist
For bug fixes, new features & breaking changes
yarn changesetand documented my changesFor new components
🧪 How to test changes