🐛 (explorer) merge grapher configs correctly (#4091)

baker/siteRenderers.tsx

@@ -826,12 +826,12 @@ const getExplorerTitleByUrl = async (
     if (url.queryStr) {
         explorer.initDecisionMatrix(url.queryParams as ExplorerFullQueryParams)
         return (
-            explorer.grapherConfig.title ??
-            (explorer.grapherConfig.grapherId
+            explorer.explorerGrapherConfig.title ??
+            (explorer.explorerGrapherConfig.grapherId
                 ? (
                       await getEnrichedChartById(
                           knex,
-                          explorer.grapherConfig.grapherId
+                          explorer.explorerGrapherConfig.grapherId
                       )
                   )?.config?.title
                 : undefined)

explorer/Explorer.tsx

@@ -9,7 +9,6 @@ import {
     GrapherInterface,
     GrapherQueryParams,
     GrapherTabOption,
-    AxisMinMaxValueStr,
 } from "@ourworldindata/types"
 import {
     OwidTable,
@@ -41,6 +40,7 @@ import {
     isInIFrame,
     keyBy,
     keyMap,
+    mergeGrapherConfigs,
     omitUndefinedValues,
     parseIntOrUndefined,
     PromiseCache,
@@ -373,7 +373,7 @@ export class Explorer
             reaction(
                 () => [
                     this.entityPickerMetric,
-                    this.explorerProgram.grapherConfig.tableSlug,
+                    this.explorerProgram.explorerGrapherConfig.tableSlug,
                 ],
                 () => this.updateEntityPickerTable()
             )
@@ -474,35 +474,6 @@ export class Explorer
         }
     }
 
-    @action.bound private updateGrapherFromExplorerCommon() {
-        const grapher = this.grapher
-        if (!grapher) return
-        const {
-            yScaleToggle,
-            yAxisMin,
-            facetYDomain,
-            relatedQuestionText,
-            relatedQuestionUrl,
-            mapTargetTime,
-        } = this.explorerProgram.grapherConfig
-
-        grapher.yAxis.canChangeScaleType = yScaleToggle
-        grapher.yAxis.min =
-            yAxisMin === AxisMinMaxValueStr.auto ? Infinity : yAxisMin
-        if (facetYDomain) {
-            grapher.yAxis.facetDomain = facetYDomain
-        }
-        if (relatedQuestionText && relatedQuestionUrl) {
-            grapher.relatedQuestions = [
-                { text: relatedQuestionText, url: relatedQuestionUrl },
-            ]
-        }
-        if (mapTargetTime) {
-            grapher.map.time = mapTargetTime
-        }
-        if (!grapher.id) grapher.id = 0
-    }
-
     @computed private get columnDefsWithoutTableSlugByIdOrSlug(): Record<
         number | string,
         OwidColumnDef
@@ -553,12 +524,14 @@ export class Explorer
         const grapher = this.grapher
         if (!grapher) return
 
-        const { grapherId } = this.explorerProgram.grapherConfig
+        const { grapherId } = this.explorerProgram.explorerGrapherConfig
         const grapherConfig = this.grapherConfigs.get(grapherId!) ?? {}
 
         const config: GrapherProgrammaticInterface = {
-            ...grapherConfig,
-            ...this.explorerProgram.grapherConfigOnlyGrapherProps,
+            ...mergeGrapherConfigs(
+                grapherConfig,
+                this.explorerProgram.grapherConfig
+            ),
             bakedGrapherURL: BAKED_GRAPHER_URL,
             dataApiUrl: DATA_API_URL,
             hideEntityControls: this.showExplorerControls,
@@ -572,9 +545,7 @@ export class Explorer
 
         grapher.setAuthoredVersion(config)
         grapher.reset()
-        this.updateGrapherFromExplorerCommon()
         grapher.updateFromObject(config)
-        grapher.slug = undefined
         grapher.downloadData()
     }
 
@@ -590,7 +561,7 @@ export class Explorer
             xSlug,
             colorSlug,
             sizeSlug,
-        } = this.explorerProgram.grapherConfig
+        } = this.explorerProgram.explorerGrapherConfig
 
         const yVariableIdsList = yVariableIds
             .split(" ")
@@ -602,8 +573,10 @@ export class Explorer
             {}
 
         const config: GrapherProgrammaticInterface = {
-            ...partialGrapherConfig,
-            ...this.explorerProgram.grapherConfigOnlyGrapherProps,
+            ...mergeGrapherConfigs(
+                partialGrapherConfig,
+                this.explorerProgram.grapherConfig
+            ),
             bakedGrapherURL: BAKED_GRAPHER_URL,
             dataApiUrl: DATA_API_URL,
             hideEntityControls: this.showExplorerControls,
@@ -689,7 +662,7 @@ export class Explorer
         }
 
         config.dimensions = dimensions
-        if (config.ySlugs && yVariableIds) config.ySlugs += " " + yVariableIds
+        if (ySlugs && yVariableIds) config.ySlugs = ySlugs + " " + yVariableIds
 
         const inputTableTransformer = (table: OwidTable) => {
             // add transformed (and intermediate) columns to the grapher table
@@ -735,7 +708,6 @@ export class Explorer
 
         grapher.setAuthoredVersion(config)
         grapher.reset()
-        this.updateGrapherFromExplorerCommon()
         grapher.updateFromObject(config)
         if (dimensions.length === 0) {
             // If dimensions are empty, explicitly set the table to an empty table
@@ -751,10 +723,10 @@ export class Explorer
     @action.bound private updateGrapherFromExplorerUsingColumnSlugs() {
         const grapher = this.grapher
         if (!grapher) return
-        const { tableSlug } = this.explorerProgram.grapherConfig
+        const { tableSlug } = this.explorerProgram.explorerGrapherConfig
 
         const config: GrapherProgrammaticInterface = {
-            ...this.explorerProgram.grapherConfigOnlyGrapherProps,
+            ...this.explorerProgram.grapherConfig,
             bakedGrapherURL: BAKED_GRAPHER_URL,
             dataApiUrl: DATA_API_URL,
             hideEntityControls: this.showExplorerControls,
@@ -768,7 +740,6 @@ export class Explorer
 
         grapher.setAuthoredVersion(config)
         grapher.reset()
-        this.updateGrapherFromExplorerCommon()
         grapher.updateFromObject(config)
 
         // Clear any error messages, they are likely to be related to dataset loading.
@@ -1098,7 +1069,7 @@ export class Explorer
         // so that when we start sorting by entity name we can infer that the column is a string column immediately.
         const tableSlugToLoad = this.entityPickerMetric
             ? this.getTableSlugOfColumnSlug(this.entityPickerMetric)
-            : this.explorerProgram.grapherConfig.tableSlug
+            : this.explorerProgram.explorerGrapherConfig.tableSlug
 
         this.entityPickerTableIsLoading = true
         void this.futureEntityPickerTable.set(
@@ -1134,7 +1105,8 @@ export class Explorer
 
         // In most cases, column slugs will be duplicated in the tables, e.g. entityName.
         // Prefer the current Grapher table if it contains the column slug.
-        const grapherTableSlug = this.explorerProgram.grapherConfig.tableSlug
+        const grapherTableSlug =
+            this.explorerProgram.explorerGrapherConfig.tableSlug
         if (
             this.tableSlugHasColumnSlug(
                 grapherTableSlug,

explorer/ExplorerProgram.test.ts

@@ -108,8 +108,8 @@ columns\ttable1\ttable2\ttable3
     describe("grapherconfig", () => {
         it("can return a grapher config", () => {
             expect(
-                new ExplorerProgram("test", `yScaleToggle\ttrue`).grapherConfig
-                    .yScaleToggle
+                new ExplorerProgram("test", `yScaleToggle\ttrue`)
+                    .explorerGrapherConfig.yScaleToggle
             ).toEqual(true)
             const program = new ExplorerProgram(
                 "test",
@@ -118,7 +118,7 @@ columns\ttable1\ttable2\ttable3
 \ttrue\tLine`
             )
             expect(program.currentlySelectedGrapherRow).toEqual(2)
-            expect(program.grapherConfig.yScaleToggle).toEqual(true)
+            expect(program.explorerGrapherConfig.yScaleToggle).toEqual(true)
         })
 
         it("can convert \\n to a newline", () => {
@@ -128,7 +128,7 @@ columns\ttable1\ttable2\ttable3
 \tsubtitle\tLine Checkbox
 \tThis is a\\ntwo-line subtitle\tLine`
             )
-            expect(program.grapherConfig.subtitle).toEqual(
+            expect(program.explorerGrapherConfig.subtitle).toEqual(
                 "This is a\ntwo-line subtitle"
             )
         })
@@ -142,9 +142,24 @@ graphers
 \tyScaleToggle\tLine Checkbox
 \ttrue\tLine`
             )
-            expect(program.grapherConfig.hasMapTab).toEqual(true)
+            expect(program.explorerGrapherConfig.hasMapTab).toEqual(true)
             // Only parse white listed grapher props
-            expect((program.grapherConfig as any).table).toEqual(undefined)
+            expect((program.explorerGrapherConfig as any).table).toEqual(
+                undefined
+            )
+        })
+
+        it("can translate explorer-specific settings to valid config fields", () => {
+            const program = new ExplorerProgram(
+                "test",
+                `hasMapTab\ttrue
+table\tfoo
+graphers
+\tyAxisMin\tLine Checkbox
+\t1\tLine`
+            )
+            expect(program.explorerGrapherConfig.yAxisMin).toEqual(1)
+            expect(program.grapherConfig.yAxis?.min).toEqual(1)
         })
     })
 

explorer/ExplorerProgram.ts

@@ -19,7 +19,7 @@ import {
     PromiseCache,
     SerializedGridProgram,
     trimObject,
-    omit,
+    merge,
 } from "@ourworldindata/utils"
 import {
     CellDef,
@@ -40,6 +40,7 @@ import {
 import { DecisionMatrix } from "./ExplorerDecisionMatrix.js"
 import { ExplorerGrammar } from "./ExplorerGrammar.js"
 import { GrapherGrammar } from "./GrapherGrammar.js"
+import { defaultGrapherConfig } from "@ourworldindata/grapher"
 
 export const EXPLORER_FILE_SUFFIX = ".explorer.tsv"
 
@@ -242,8 +243,8 @@ export class ExplorerProgram extends GridProgram {
     // that use Grapher IDs as well as CSV data files to create charts,
     // but we plan to drop support for mixed-content explorers in the future
     get chartCreationMode(): ExplorerChartCreationMode {
-        const { decisionMatrix, grapherConfig } = this
-        const { grapherId } = grapherConfig
+        const { decisionMatrix, explorerGrapherConfig } = this
+        const { grapherId } = explorerGrapherConfig
         const yVariableIdsColumn = decisionMatrix.table.get(
             GrapherGrammar.yVariableIds.keyword
         )
@@ -359,29 +360,56 @@ export class ExplorerProgram extends GridProgram {
         return clone
     }
 
-    get grapherConfig(): ExplorerGrapherInterface {
+    /**
+     * Grapher config for the currently selected row including global settings.
+     * Includes all columns that are part of the GrapherGrammar.
+     */
+    get explorerGrapherConfig(): ExplorerGrapherInterface {
         const rootObject = trimAndParseObject(this.tuplesObject, GrapherGrammar)
-
-        Object.keys(rootObject).forEach((key) => {
-            if (!GrapherGrammar[key]) delete rootObject[key]
-        })
+        let config = { ...rootObject }
 
         const selectedGrapherRow = this.decisionMatrix.selectedRow
         if (selectedGrapherRow && Object.keys(selectedGrapherRow).length) {
-            return { ...rootObject, ...selectedGrapherRow }
+            config = { ...config, ...selectedGrapherRow }
         }
 
-        return rootObject
+        // remove all keys that are not part of the GrapherGrammar
+        Object.keys(config).forEach((key) => {
+            if (!GrapherGrammar[key]) delete config[key]
+        })
+
+        return config
     }
 
-    get grapherConfigOnlyGrapherProps() {
-        return omit(this.grapherConfig, [
-            GrapherGrammar.yVariableIds.keyword,
-            GrapherGrammar.xVariableId.keyword,
-            GrapherGrammar.colorVariableId.keyword,
-            GrapherGrammar.sizeVariableId.keyword,
-            GrapherGrammar.mapTargetTime.keyword,
-        ])
+    /**
+     * Grapher config for the currently selected row, with explorer-specific
+     * fields translated to valid GrapherInterface fields.
+     *
+     * For example, `yAxisMin` is translated to `{yAxis: {min: ... }}`
+     */
+    get grapherConfig(): GrapherInterface {
+        const partialConfigs: GrapherInterface[] = []
+        const fields = Object.entries(this.explorerGrapherConfig)
+        for (const [field, value] of fields) {
+            const cellDef = GrapherGrammar[field]
+            partialConfigs.push(cellDef.toGrapherObject(value))
+        }
+
+        const mergedConfig = merge({}, ...partialConfigs)
+
+        // assume config is valid against the latest schema
+        mergedConfig.$schema = defaultGrapherConfig.$schema
+
+        // TODO: can be removed once relatedQuestions is refactored
+        const { relatedQuestionUrl, relatedQuestionText } =
+            this.explorerGrapherConfig
+        if (relatedQuestionUrl && relatedQuestionText) {
+            mergedConfig.relatedQuestions = [
+                { url: relatedQuestionUrl, text: relatedQuestionText },
+            ]
+        }
+
+        return mergedConfig
     }
 
     /**