owid-grapher icon indicating copy to clipboard operation
owid-grapher copied to clipboard

Explicitly pass client settings into Grapher

Open danyx23 opened this issue 2 years ago • 1 comments

Core problem

Client settings (defined in settings/clientSettings.ts are currently made available for Grapher by making webpack include the contents at build time. This has a number of downsides:

  • Settings that depend on environment variables are only evaluated when webpack runs (e.g. if you want to change where the admin grapher instances should get their data from you can't just change a .env file and restart the admin server)
  • It is not easy to change these settings for users who want to use Grapher as a react component outside of our project
  • We want to make it easier to translate UI strings that are used in grapher (#1539) - the client settings could be a convenient place where these strings could live so that they are easy to change.

Proposed solution

Stop shipping clientsettings via webpack. Instead create a type for our settings and an instance with default values and extend the props of Grapher to take those, then pass them down to wherever they are used. We could also use context for this but less magic is probably better.

The Grapher instance would then in the constructor fill whatever is missing in the the passed settings with the default settings.

The default settings should maybe be baked into our standalone grapher pages. This would be weird on it's own but the approach some people from wikimedia have been taking would probably benefit from doing this (although we should check with them, maybe just having a settings object that is empty is good enough for them to work with.

One open question is what to do with constructed UI strings like "Add ENTITY" (where entity is replaced at runtime, usually with the label country) - but we can probably come up with a syntax for this.

Alternatives

Only do this for specific properties that need it (like I did in #1599 which was a motivating example for this change).

danyx23 avatar Aug 17 '22 10:08 danyx23

Currently clientSettings contains:

BUGSNAG_API_KEY
ADMIN_SERVER_PORT
ADMIN_SERVER_HOST
BAKED_BASE_URL
BAKED_GRAPHER_URL
BAKED_GRAPHER_EXPORTS_BASE_URL
ADMIN_BASE_URL
WORDPRESS_URL
ALGOLIA_ID
ALGOLIA_SEARCH_KEY
STRIPE_PUBLIC_KEY
DONATE_API_URL
RECAPTCHA_SITE_KEY
TOPICS_CONTENT_GRAPH

Of these, Grapher uses ADMIN_BASE_URL and BAKED_GRAPHER_URL - both of which are also used in other parts of the project (SVG Tester, Admin Client, etc) so they'd have to exist in two places. I can see this leading to gotchas where someone updates their env file to test something but forgets to rebuild the rest of the stack.

Something to keep in mind 🤷

ikesau avatar Aug 22 '22 19:08 ikesau

This issue has had no activity within 10 months. It is considered stale and will be closed in 7 days unless it is worked on or tagged as pinned.

github-actions[bot] avatar Aug 07 '23 07:08 github-actions[bot]

@marcelgerber have we done this as part of the vite changes?

danyx23 avatar Aug 07 '23 10:08 danyx23

No, we didn't. In vite we're pretty much using the same settings flow as we had in webpack.

marcelgerber avatar Aug 07 '23 11:08 marcelgerber

This issue has had no activity within 10 months. It is considered stale and will be closed in 7 days unless it is worked on or tagged as pinned.

github-actions[bot] avatar Jun 03 '24 07:06 github-actions[bot]