Skip to content

Commit

Permalink
🔨 add empty arrays to the default grapher config (#4294)
Browse files Browse the repository at this point in the history
Empty arrays are not currently included in Grapher's default config object that is derived from the schema. That's a bit of a foot gun, so I added them now.

For context: When I implemented inheritance, I decided to keep empty arrays out of the default config since all defaults ended up in the `fullConfig` of each chart and that would have been unnecessarily verbose. But now that we're not doing that anymore, I can't think of a downside of also including the default values for arrays.

This has two nice effects:
- It used to be important that Grapher serialises empty arrays, so that a parent value can be overwritten with that empty array (e.g. if a parent has a related question that a child doesn't need). Now we can rely on these values being present in the default config.
- Another nice effect is that we can simplify a number of tests like this one where `selectedEntityNames` was always serialised, even for an empty Grapher:

https://github.com/owid/owid-grapher/blob/28ba2e4c036d0eb6156a4609add48f9e6077a764/packages/%40ourworldindata/grapher/src/core/Grapher.jsdom.test.ts#L92-L98
  • Loading branch information
sophiamersmann authored Dec 17, 2024
1 parent 2e6b62d commit 0769513
Show file tree
Hide file tree
Showing 7 changed files with 36 additions and 25 deletions.
2 changes: 1 addition & 1 deletion adminSiteClient/AbstractChartEditor.ts
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ export abstract class AbstractChartEditor<
/** patch config merged with parent config */
@computed get fullConfig(): GrapherInterface {
if (!this.activeParentConfig) return this.liveConfig
return mergeGrapherConfigs(this.activeParentConfig, this.liveConfig)
return mergeGrapherConfigs(this.activeParentConfig, this.patchConfig)
}

/** parent config currently applied to grapher */
Expand Down
20 changes: 11 additions & 9 deletions devTools/schema/generate-default-object-from-schema.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@ import parseArgs from "minimist"
import fs from "fs-extra"
import { range } from "lodash"

const TEMPLATE_FILENAME = "./devTools/schema/template"

const schemaVersionRegex =
/https:\/\/files\.ourworldindata\.org\/schemas\/grapher-schema\.(?<version>\d{3})\.json/m
const getSchemaVersion = (config: Record<string, any>): string =>
Expand Down Expand Up @@ -58,23 +60,23 @@ async function main(parsedArgs: parseArgs.ParsedArgs) {

// save as ts file if requested
if (parsedArgs["save-ts"]) {
const template = fs.readFileSync(TEMPLATE_FILENAME, "utf8")

const latestVersion = getSchemaVersion(defaultConfig)
const outdatedVersionsAsInts = range(1, parseInt(latestVersion))
const outdatedVersions = outdatedVersionsAsInts.map((versionNumber) =>
versionNumber.toString().padStart(3, "0")
)

const out = parsedArgs["save-ts"]
const content = `// THIS IS A GENERATED FILE, DO NOT EDIT DIRECTLY
// GENERATED BY devTools/schema/generate-default-object-from-schema.ts
import { GrapherInterface } from "@ourworldindata/types"
export const latestSchemaVersion = "${latestVersion}" as const
export const outdatedSchemaVersions = ${toArrayString(outdatedVersions)} as const
const content = template
.replace("{{LATEST_SCHEMA_VERSION}}", latestVersion.toString())
.replace(
"{{OUTDATED_SCHEMA_VERSIONS}}",
toArrayString(outdatedVersions)
)
.replace("{{DEFAULT_GRAPHER_CONFIG}}", defaultConfigJSON)

export const defaultGrapherConfig = ${defaultConfigJSON} as GrapherInterface`
fs.outputFileSync(out, content)
}

Expand Down
10 changes: 10 additions & 0 deletions devTools/schema/template
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
// THIS IS A GENERATED FILE, DO NOT EDIT DIRECTLY

// GENERATED BY devTools/schema/generate-default-object-from-schema.ts

import { GrapherInterface } from "@ourworldindata/types"

export const latestSchemaVersion = "{{LATEST_SCHEMA_VERSION}}" as const
export const outdatedSchemaVersions = {{OUTDATED_SCHEMA_VERSIONS}} as const

export const defaultGrapherConfig = {{DEFAULT_GRAPHER_CONFIG}} as GrapherInterface
12 changes: 0 additions & 12 deletions packages/@ourworldindata/grapher/src/core/Grapher.jsdom.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -79,10 +79,6 @@ it("can get dimension slots", () => {
it("an empty Grapher serializes to an object that includes only the schema", () => {
expect(new Grapher().toObject()).toEqual({
$schema: latestGrapherConfigSchema,

// TODO: ideally, these are not serialised for an empty object
selectedEntityNames: [],
focusedSeriesNames: [],
})
})

Expand All @@ -93,20 +89,12 @@ it("a bad chart type does not crash grapher", () => {
expect(new Grapher(input).toObject()).toEqual({
...input,
$schema: latestGrapherConfigSchema,

// TODO: ideally, these are not serialised for an empty object
selectedEntityNames: [],
focusedSeriesNames: [],
})
})

it("does not preserve defaults in the object (except for the schema)", () => {
expect(new Grapher({ tab: GRAPHER_TAB_OPTIONS.chart }).toObject()).toEqual({
$schema: latestGrapherConfigSchema,

// TODO: ideally, these are not serialised for an empty object
selectedEntityNames: [],
focusedSeriesNames: [],
})
})

Expand Down
5 changes: 2 additions & 3 deletions packages/@ourworldindata/grapher/src/core/Grapher.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -437,15 +437,14 @@ export class Grapher
[entityName: string]: string | undefined
} = {}

// Initializing arrays with `undefined` ensures that empty arrays get serialised
@observable selectedEntityNames?: EntityName[] = undefined
@observable selectedEntityNames: EntityName[] = []
@observable focusedSeriesNames: SeriesName[] = []
@observable excludedEntities?: number[] = undefined
/** IncludedEntities are usually empty which means use all available entities. When
includedEntities is set it means "only use these entities". excludedEntities
are evaluated afterwards and can still remove entities even if they were included before.
*/
@observable includedEntities?: number[] = undefined
@observable focusedSeriesNames?: SeriesName[] = undefined
@observable comparisonLines?: ComparisonLineConfig[] = undefined // todo: Persistables?
@observable relatedQuestions?: RelatedQuestionsConfig[] = undefined // todo: Persistables?

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,8 @@ export const defaultGrapherConfig = {
time: "latest",
},
maxTime: "latest",
selectedEntityNames: [],
focusedSeriesNames: [],
yAxis: {
removePointsOutsideDomain: false,
scaleType: "linear",
Expand All @@ -54,11 +56,13 @@ export const defaultGrapherConfig = {
selectedFacetStrategy: "none",
invertColorScheme: false,
hideRelativeToggle: true,
comparisonLines: [],
logo: "owid",
entityType: "country or region",
facettingLabelByYVariables: "metric",
addCountryMode: "add-country",
compareEndPointsOnly: false,
relatedQuestions: [],
chartTypes: ["LineChart"],
hasMapTab: false,
stackMode: "absolute",
Expand All @@ -68,6 +72,7 @@ export const defaultGrapherConfig = {
time: false,
changeInPrefix: false,
},
excludedEntities: [],
xAxis: {
removePointsOutsideDomain: false,
scaleType: "linear",
Expand All @@ -85,6 +90,7 @@ export const defaultGrapherConfig = {
sortBy: "total",
sortOrder: "desc",
hideFacetControl: true,
includedEntities: [],
entityTypePlural: "countries and regions",
missingDataStrategy: "auto",
} as GrapherInterface
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,7 @@ properties:
selectedEntityNames:
type: array
description: The initial selection of entities
default: []
items:
type:
- string
Expand All @@ -109,6 +110,7 @@ properties:
description: |
The initially focused chart elements. Is either a list of entity or variable names.
Only works for line and slope charts for now.
default: []
items:
type:
- string
Expand Down Expand Up @@ -195,6 +197,7 @@ properties:
comparisonLines:
description: List of vertical comparison lines to draw
type: array
default: []
items:
type: object
properties:
Expand Down Expand Up @@ -361,6 +364,7 @@ properties:
relatedQuestions:
type: array
description: Links to related questions
default: []
items:
type: object
properties:
Expand Down Expand Up @@ -428,6 +432,7 @@ properties:
excludedEntities:
type: array
description: Entities that should be excluded from the chart
default: []
items:
type: integer
minimum: 0
Expand Down Expand Up @@ -493,6 +498,7 @@ properties:
includedEntities:
type: array
description: Entities to include. Opposite of excludedEntities. If this is set then all entities not specified here are excluded.
default: []
items:
type: number
entityTypePlural:
Expand Down

0 comments on commit 0769513

Please sign in to comment.