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

🔨 Fix grapher uses after grapher state refactor [4/4] #4419

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

danyx23
Copy link
Contributor

@danyx23 danyx23 commented Jan 9, 2025

This is the second big PR in the stack, the one that tries to fix things again. This PR attempts to make Grapher useable again in all places after the state was extracted into GrapherState and the automatic data loading was taken out in favour of explicit functions for loading and preparing the input table.

Most of the internal MobX logic that was in Grapher is now extracted into a utity class GrapherState. Grapher takes a GrapherState instance as it's only prop - the idea is that react code at some point creates a new GrapherState and keeps it around in a ref and just passes that one as a prop to concrete Grapher components. Eventually, Grapher might even just be a functional component, but there are still a few properties and actions and so on on the Grapher class in this PR.

The GrapherState instance is also what is passed as manager into various other components to control aspects of them and it is what is now used in the Grapher tests.

The various chart instances (LineChart, ...) are not yet separated from their state. I think it would be nice to give them the same treatment that Grapher got (extract all their mobx state into a utility class and make the actual React component very lightweight) but this PR is already so huge...

As for using Grapher on our site, the basic idea with this PR is to have the following component hierarchy (from most comfortable to use to most basic one):

  • GrapherWithFallback - shows a GrapherImage thumbnail on server render, on client render uses an interaction observer to initialize a GrapherFigureView with a GrapherImage as fallback.
  • GrapherFigureView - measures the size of the containing element and updated FetchingGrapher accordingly
  • FetchingGrapher - takes care of loading variable data and metadata files and initializing the InputTable
  • Grapher - just the Grapher that doesn't load data. It needs the inputTable to be set from an external user.

One spiritual goal of this PR stack is to get rid of the Multiembedder and just use GrapherWithFallback to take care of the interaction observers and so on. This PR takes a first step into that direction in that the Chart gdoc component now uses a GrapherWithFallback instead of rendering a figure that is then replaced by the Multiembedder, but it does not do this for a bunch of other cases (e.g. explorers, Mdims, ...) or on pages that are not yet properly hydrated like the old standalone Grapher pages.

Known blockers

(please add more as you find them)

  • 2 tests are failing
  • the explorer.jsdom.test.tsx tests don't run (owidDataset isn't properly loaded in SampleExplorerOfGraphers)
  • In the admin the chart is too small
  • Several SVG tests are failing - it looks like this is because of the font size used being different. Maybe this is related to the above issue?
  • The Chart component can have customizations (through the query string) that are not yet passed down through the updated component hierarchy.

Open questions / things to check

  • The Admin used to have a separate data fetch codepath. I don't think we still need this and have removed it but this should be verified again.
  • Because the embedding components (GrapherWithFallback, ...) changed quite a bit, there might be a lot of remaining smaller issues around the site when Grapher is used.
  • In the CF functions, I've added DATA_API_URL since we now need this when fetching the input table. I'm not sure if the env is the best way to do this.
  • The way the GRAPHER_BASE_URL and DATA_API_URL are passed through have changed quite a bit. Need to check that this still works correctly for scenarios like iframe embeds, staging servers, ...
  • authorsVersion and initialOptions do almost the same thing now - can they be unified?

Steps to discuss for after this is merged

  • Should GrapherState be split once more? There seems to be a breaking point of everything declared in GrapherProgrammaticInterface and then all the properties computed from that. Is it good enough if we rely on the type system to hide implementation detail fields of GrapherState or would we prefer to have one class that is the grapher API that can be manipulated and another one that then derives all the other state from that?

@danyx23 danyx23 changed the base branch from master to extract-grapher-state January 9, 2025 10:41
@danyx23 danyx23 force-pushed the extract-grapher-state branch 2 times, most recently from 70ad31c to 2348bca Compare January 10, 2025 08:46
@danyx23 danyx23 force-pushed the fix-grapher-uses branch 2 times, most recently from 41c0837 to e28bfc1 Compare January 10, 2025 17:45
@danyx23 danyx23 force-pushed the extract-grapher-state branch from 2348bca to e22a5b5 Compare January 10, 2025 17:45
@danyx23 danyx23 force-pushed the extract-grapher-state branch from e22a5b5 to d4e1269 Compare January 10, 2025 17:50
@danyx23 danyx23 force-pushed the fix-grapher-uses branch 2 times, most recently from faf3b7c to 4325f5c Compare January 11, 2025 15:36
@danyx23 danyx23 force-pushed the extract-grapher-state branch from 530ed25 to f97f5e8 Compare January 22, 2025 16:13
@danyx23 danyx23 force-pushed the extract-grapher-state branch from f97f5e8 to 954bcac Compare February 1, 2025 18:38
@danyx23 danyx23 force-pushed the fix-grapher-uses branch 2 times, most recently from 2d53d5a to 9d1ae90 Compare February 2, 2025 16:47
@danyx23 danyx23 force-pushed the extract-grapher-state branch from 954bcac to f859fe0 Compare February 3, 2025 08:26
@danyx23 danyx23 force-pushed the extract-grapher-state branch from f859fe0 to ab5227e Compare February 3, 2025 08:37
@danyx23 danyx23 force-pushed the fix-grapher-uses branch 2 times, most recently from 9358595 to 1f83a0a Compare February 3, 2025 08:55
@danyx23 danyx23 marked this pull request as ready for review February 3, 2025 09:59
@marcelgerber marcelgerber changed the title 🔨 Fix grapher uses after grapher state refactor 🔨 Fix grapher uses after grapher state refactor [4/4] Feb 5, 2025
@danyx23 danyx23 changed the base branch from extract-grapher-state to master February 5, 2025 20:03
grapher.seriesColorMap?.clear()
grapher.rebuildInputOwidTable()
grapherState.seriesColorMap?.clear()
// TODO: 2025-01-05 Daniel we used to rebuild the table here but that

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A new Todo was discovered. If it is not a priority right now,consider marking it for later attention.
TODO: 2025-01-05 Daniel we used to rebuild the table here but that

  • Ignore

fetchConfigForSlug?: boolean
}

// TODO: change this so it's possible to hand a full grapher config down (and maybe an extra config?)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A new Todo was discovered. If it is not a priority right now,consider marking it for later attention.
TODO: change this so it's possible to hand a full grapher config down (and maybe an extra config?)

  • Ignore

@danyx23 danyx23 force-pushed the fix-grapher-uses branch 4 times, most recently from a50afba to 2a94e58 Compare February 21, 2025 13:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant