🔨 refactor legacyToOwidTableAndDimensions to be chart type specific

packages/@ourworldindata/grapher/src/core/Grapher.tsx

@@ -1145,10 +1145,11 @@ export class Grapher
         // TODO grapher model: switch this to downloading multiple data and metadata files
 
         const startMark = performance.now()
-        const { dimensions, table } = legacyToOwidTableAndDimensions(
+        const table = legacyToOwidTableAndDimensions(
             json,
-            legacyConfig
+            legacyConfig.dimensions ?? []
         )
+        const dimensions = legacyConfig.dimensions ?? []
         this.createPerformanceMeasurement(
             "legacyToOwidTableAndDimensions",
             startMark

packages/@ourworldindata/grapher/src/core/LegacyToOwidTable.test.ts

@@ -8,7 +8,10 @@ import {
     LegacyGrapherInterface,
 } from "@ourworldindata/types"
 import { ColumnTypeMap, ErrorValueTypes } from "@ourworldindata/core-table"
-import { legacyToOwidTableAndDimensions } from "./LegacyToOwidTable"
+import {
+    addSelectedEntityColorsToTable,
+    legacyToOwidTableAndDimensions,
+} from "./LegacyToOwidTable"
 import {
     MultipleOwidVariableDataDimensionsMap,
     OwidVariableDataMetadataDimensions,
@@ -43,9 +46,9 @@ describe(legacyToOwidTableAndDimensions, () => {
     }
 
     it("contains the standard entity columns", () => {
-        const { table } = legacyToOwidTableAndDimensions(
+        const table = legacyToOwidTableAndDimensions(
             legacyVariableConfig,
-            legacyGrapherConfig
+            legacyGrapherConfig.dimensions ?? []
         )
         expect(table.columnSlugs).toEqual(
             expect.arrayContaining(
@@ -62,27 +65,22 @@ describe(legacyToOwidTableAndDimensions, () => {
 
     describe("conversionFactor", () => {
         it("applies the more specific chart-level conversionFactor", () => {
-            const { table } = legacyToOwidTableAndDimensions(
-                legacyVariableConfig,
+            const table = legacyToOwidTableAndDimensions(legacyVariableConfig, [
                 {
-                    dimensions: [
-                        {
-                            variableId: 2,
-                            display: { conversionFactor: 10 },
-                            property: DimensionProperty.y,
-                        },
-                    ],
-                }
-            )
+                    variableId: 2,
+                    display: { conversionFactor: 10 },
+                    property: DimensionProperty.y,
+                },
+            ])
 
             // Apply the chart-level conversionFactor (10)
             expect(table.rows[0]["2"]).toEqual(80)
         })
 
         it("applies the more variable-level conversionFactor if a chart-level one is not present", () => {
-            const { table } = legacyToOwidTableAndDimensions(
+            const table = legacyToOwidTableAndDimensions(
                 legacyVariableConfig,
-                legacyGrapherConfig
+                legacyGrapherConfig.dimensions ?? []
             )
 
             // Apply the variable-level conversionFactor (100)
@@ -172,9 +170,9 @@ describe(legacyToOwidTableAndDimensions, () => {
             ],
         }
 
-        const { table } = legacyToOwidTableAndDimensions(
+        const table = legacyToOwidTableAndDimensions(
             legacyVariableConfig,
-            legacyGrapherConfig
+            legacyGrapherConfig.dimensions ?? []
         )
 
         it("leaves ErrorValues when there were no values to join to", () => {
@@ -315,9 +313,9 @@ describe(legacyToOwidTableAndDimensions, () => {
             ],
         }
 
-        const { table } = legacyToOwidTableAndDimensions(
+        const table = legacyToOwidTableAndDimensions(
             legacyVariableConfig,
-            legacyGrapherConfig
+            legacyGrapherConfig.dimensions ?? []
         )
 
         it("shifts values in days array when zeroDay is is not EPOCH_DATE", () => {
@@ -438,9 +436,9 @@ describe(legacyToOwidTableAndDimensions, () => {
             ],
         }
 
-        const { table } = legacyToOwidTableAndDimensions(
+        const table = legacyToOwidTableAndDimensions(
             legacyVariableConfig,
-            legacyGrapherConfig
+            legacyGrapherConfig.dimensions ?? []
         )
 
         it("duplicates 'day' column into 'time'", () => {
@@ -469,9 +467,9 @@ describe(legacyToOwidTableAndDimensions, () => {
                     chartTypes: [GRAPHER_CHART_TYPES.ScatterPlot],
                 }
 
-                const { table } = legacyToOwidTableAndDimensions(
+                const table = legacyToOwidTableAndDimensions(
                     legacyVariableConfig,
-                    scatterLegacyGrapherConfig
+                    scatterLegacyGrapherConfig.dimensions ?? []
                 )
 
                 expect(table.rows.length).toEqual(3)
@@ -627,9 +625,9 @@ describe("variables with mixed days & years with missing overlap and multiple po
         ],
     }
 
-    const { table } = legacyToOwidTableAndDimensions(
+    const table = legacyToOwidTableAndDimensions(
         legacyVariableConfig,
-        legacyGrapherConfig
+        legacyGrapherConfig.dimensions ?? []
     )
 
     it("duplicates 'day' column into 'time'", () => {
@@ -646,9 +644,9 @@ describe("variables with mixed days & years with missing overlap and multiple po
 
     describe("join behaviour without target times is sane", () => {
         it("creates a sane table join", () => {
-            const { table } = legacyToOwidTableAndDimensions(
+            const table = legacyToOwidTableAndDimensions(
                 legacyVariableConfig,
-                legacyGrapherConfig
+                legacyGrapherConfig.dimensions ?? []
             )
 
             // A sane join between years and days would create 5 days for the given input
@@ -764,10 +762,18 @@ const getLegacyGrapherConfig = (): Partial<LegacyGrapherInterface> => {
 }
 
 describe("creating a table from legacy", () => {
-    const table = legacyToOwidTableAndDimensions(getOwidVarSet(), {
+    const config = {
         ...getLegacyGrapherConfig(),
         selectedEntityColors: { "Cape Verde": "blue" },
-    }).table
+    }
+    const table = addSelectedEntityColorsToTable(
+        legacyToOwidTableAndDimensions(
+            getOwidVarSet(),
+            config.dimensions ?? []
+        ),
+        config.selectedEntityColors
+    )
+
     const name =
         "Prevalence of wasting, weight for height (% of children under 5)"
 
@@ -802,8 +808,8 @@ describe("creating a table from legacy", () => {
         expect(
             legacyToOwidTableAndDimensions(
                 varSet,
-                getLegacyGrapherConfig()
-            ).table.get("3512")!.values
+                getLegacyGrapherConfig().dimensions ?? []
+            ).get("3512")!.values
         ).toEqual([550, 420, 1260])
     })
 
@@ -825,8 +831,8 @@ Papua New Guinea,PNG,1983,5.5`
         varSet.get(3512)!.metadata.nonRedistributable = true
         const columnDef = legacyToOwidTableAndDimensions(
             varSet,
-            getLegacyGrapherConfig()
-        ).table.get("3512").def as OwidColumnDef
+            getLegacyGrapherConfig().dimensions ?? []
+        ).get("3512").def as OwidColumnDef
         expect(columnDef.nonRedistributable).toEqual(true)
     })
 })

packages/@ourworldindata/grapher/src/core/LegacyToOwidTable.ts

@@ -1,15 +1,14 @@
 // todo: Remove this file when we've migrated OWID data and OWID charts to next version
 
 import {
-    GRAPHER_CHART_TYPES,
     ColumnTypeNames,
     CoreColumnDef,
     StandardOwidColumnDefs,
     OwidTableSlugs,
     OwidColumnDef,
-    LegacyGrapherInterface,
     OwidVariableDimensions,
     OwidVariableDataMetadataDimensions,
+    EntityId,
 } from "@ourworldindata/types"
 import {
     OwidTable,
@@ -42,8 +41,8 @@ import { isContinentsVariableId } from "./GrapherConstants"
 
 export const legacyToOwidTableAndDimensions = (
     json: MultipleOwidVariableDataDimensionsMap,
-    grapherConfig: Partial<LegacyGrapherInterface>
-): { dimensions: OwidChartDimensionInterface[]; table: OwidTable } => {
+    dimensions: OwidChartDimensionInterface[]
+): OwidTable => {
     // Entity meta map
 
     const entityMeta = [...json.values()].flatMap(
@@ -53,26 +52,13 @@ export const legacyToOwidTableAndDimensions = (
         entityMeta.map((entity) => [entity.id.toString(), entity])
     )
 
-    const dimensions = grapherConfig.dimensions || []
-
     // Base column defs, shared by all variable tables
 
     const baseColumnDefs: Map<ColumnSlug, CoreColumnDef> = new Map()
     StandardOwidColumnDefs.forEach((def) => {
         baseColumnDefs.set(def.slug, def)
     })
 
-    const entityColorColumnSlug = grapherConfig.selectedEntityColors
-        ? OwidTableSlugs.entityColor
-        : undefined
-    if (entityColorColumnSlug) {
-        baseColumnDefs.set(entityColorColumnSlug, {
-            slug: entityColorColumnSlug,
-            type: ColumnTypeNames.Color,
-            name: entityColorColumnSlug,
-        })
-    }
-
     // We need to create a column for each unique [variable, targetTime] pair. So there can be
     // multiple columns for a single variable.
     const newDimensions = dimensions.map((dimension) => ({
@@ -174,18 +160,6 @@ export const legacyToOwidTableAndDimensions = (
             )
             columnDefs.set(annotationColumnDef.slug, annotationColumnDef)
         }
-
-        if (entityColorColumnSlug) {
-            columnStore[entityColorColumnSlug] = entityIds.map((entityId) => {
-                // see comment above about entityMetaById[id]
-                const entityName = entityMetaById[entityId]?.name
-                const selectedEntityColors = grapherConfig.selectedEntityColors
-                return entityName && selectedEntityColors
-                    ? selectedEntityColors[entityName]
-                    : undefined
-            })
-        }
-
         // Build the tables
 
         let variableTable = new OwidTable(
@@ -198,12 +172,7 @@ export const legacyToOwidTableAndDimensions = (
         // We do this by dropping the column. We interpolate before which adds an originalTime
         // column which can be used to recover the time.
         const targetTime = dimension?.targetYear
-        const chartType = grapherConfig.chartTypes?.[0]
-        if (
-            (chartType === GRAPHER_CHART_TYPES.ScatterPlot ||
-                chartType === GRAPHER_CHART_TYPES.Marimekko) &&
-            isNumber(targetTime)
-        ) {
+        if (isNumber(targetTime)) {
             variableTable = variableTable
                 // interpolateColumnWithTolerance() won't handle injecting times beyond the current
                 // allTimes. So if targetYear is 2018, and we have data up to 2017, the
@@ -359,7 +328,34 @@ export const legacyToOwidTableAndDimensions = (
         }
     }
 
-    return { dimensions: newDimensions, table: joinedVariablesTable }
+    return joinedVariablesTable
+}
+
+export const addSelectedEntityColorsToTable = (
+    table: OwidTable,
+    selectedEntityColors: { [entityName: string]: string }
+): OwidTable => {
+    const entityColorColumnSlug = OwidTableSlugs.entityColor
+
+    const valueFn = (entityId: EntityId | undefined) => {
+        // see comment above about entityMetaById[id]
+        if (!entityId) return ErrorValueTypes.UndefinedButShouldBeString
+        const entityName = table.entityIdToNameMap.get(entityId)
+        return entityName && selectedEntityColors
+            ? selectedEntityColors[entityName]
+            : ErrorValueTypes.UndefinedButShouldBeString
+    }
+
+    const values = table.rows.map((row) => valueFn(row.entityId))
+
+    return table.appendColumns([
+        {
+            slug: entityColorColumnSlug,
+            name: entityColorColumnSlug,
+            type: ColumnTypeNames.Color,
+            values: values,
+        },
+    ])
 }
 
 const fullJoinTables = (