Datapages are fetching same variable data multiple times

owid / owid-grapher

A platform for creating interactive data visualizations

https://ourworldindata.org

MIT License

1.36k stars 230 forks source link

Datapages are fetching same variable data multiple times #3088

Open ikesau opened 8 months ago

ikesau commented 8 months ago

When loading a datapage, multiple requests are made to load the indicator metadata.

Locally, adding a console.log to Grapher.componentDidMount shows that the component is mounting three times.

These requests are getting cached so I don't think there's a major performance impact, but we should probably fix this. I think it's responsible for a bit of jumpiness in the loading-in animation of linecharts:

https://github.com/owid/owid-grapher/assets/11844404/ade20be4-b9e4-4450-8f48-01623dd59701

ikesau commented 7 months ago

Ideally we can fix https://github.com/owid/owid-grapher/issues/2992 as part of this, too

toni-sharpe commented 6 months ago

I just had a similar one in React 18 and it was React itself: https://legacy.reactjs.org/docs/strict-mode.html

However (I can't tell from your post) it's not applied in production. In production I don't think I see a dupe (FireFox test) - I see that asset loaded once:

This solution is not related to JS being off and things not loading

rakyi commented 3 months ago

We fetch the indicator data and metadata in the Grapher constructor. The reason it happens four times on https://ourworldindata.org/grapher/life-expectancy is AFAICT because we:

Construct it in DataPageV2Content on render and pass it to two children; this is the main graph on the page
In each of the two nested GrapherFigureView children we construct it again
In RelatedCharts we construct another Grapher for the first related chart (life-expectancy-marimekko), which also happens to need the same indicator

The nice long-term solution is to dedupe/cache the API requests, either by using some kind of (meta)-framework that has this built in, or more realistically something like react-query or swr. More specifically, we could use only the QueryCache, since we can't use hooks (useQuery) in Grapher, because it's a class component.

marcelgerber commented 3 months ago

@rakyi For the main chart, why are there two <GrapherFigureView>s and three grapher instances constructed? Is there any sensible reason for this?

rakyi commented 3 months ago

I tried changing GrapherFigureView to take a grapher config as a prop instead of the whole object, and that wasn't trivial because we are accessing properties that are only on the object. So that's AFAICT the reason we create the instance in the parent.

There are two children GrapherFigureViews, because one is visible on our page and the other is there for iframe embed, but I'm not sure why we need two.