Fix multidimensional data pages baking logic #4478
Conversation
rakyi
commented
Jan 23, 2025
•
rakyi
commented
- Don't delete mdims in GrapherBaker
- Delete old mdims in MultiDimBaker
- Improve the console output
- Document issue with TSConfig and getting ESNext types
| @@ -308,7 +309,6 @@ const bakeGrapherPageAndVariablesPngAndSVGIfChanged = async ( | |||
| imageMetadataDictionary | |||
| ) | |||
| ) | |||
| console.log(outPath) | |||
There was a problem hiding this comment.
We output the slug in the progress bar already, this doesn't add anything useful on top of that.
| @@ -432,7 +431,7 @@ export const bakeAllChangedGrapherPagesVariablesPngSvgAndDeleteRemovedGraphers = | |||
| ) | |||
|
|
|||
| const progressBar = new ProgressBar( | |||
| "bake grapher page [:bar] :current/:total :elapseds :rate/s :etas :name\n", | |||
There was a problem hiding this comment.
It's not clear this is an ETA in the output, which is already cluttered, and doesn't seem useful.
| @@ -451,11 +450,18 @@ export const bakeAllChangedGrapherPagesVariablesPngSvgAndDeleteRemovedGraphers = | |||
| async (knex) => await bakeSingleGrapherChart(job, knex), | |||
| db.TransactionCloseMode.KeepOpen | |||
| ) | |||
| progressBar.tick({ name: `slug ${job.slug}` }) | |||
| progressBar.tick({ name: job.slug }) | |||
There was a problem hiding this comment.
Repeating slug on every line doesn't add anything useful.
rakyi
force-pushed
the
fix-mdim-baking-logic
branch
from
6e05aa2 to
8385cc3
Compare
|
Quick links (staging server):
Login:
SVG tester:Number of differences (default views): 0 ✅ Edited: 2025-01-23 11:44:17 UTC |
rakyi
force-pushed
the
fix-mdim-baking-logic
branch
from
8385cc3 to
2a6a3d2
Compare
rakyi
force-pushed
the
fix-mdim-baking-logic
branch
3 times, most recently
from
fab579d to
af22b53
Compare
* Don't delete mdims in GrapherBaker * Delete old mdims in MultiDimBaker * Improve the console output * Document issue with TSConfig and getting ESNext types
rakyi
force-pushed
the
fix-mdim-baking-logic
branch
from
af22b53 to
2d02649
Compare
There was a problem hiding this comment.
Nice!
My main concern is about the repetition of deleteOldGrapher / deleteOldMultiDimPages:
1- code redundancy: they largely do the same thing
2- execution redundancy: the deletion of obsolete /grapher pages happens twice (once at the grapher step, and another time at the mdim step)
2 is more of an optimzation for full bakes, and doesn't probably count for much so I wouldn't necessarily complexify here. Also it's nice that each step is self-contained when we do partial bakes.
The near duplication of deleteOldGrapher in 1 is a bit more cumbersome, and it would be more straightforward to have a single function that deals with the overall deletion of /grapher pages (even if it called twice, as per 2).
It would be also a good opportunity to deal with the confusion around redirects to explorers.
On the one hand, we don't delete grapher pages that have been redirected to explorers:
owid-grapher/baker/GrapherBaker.tsx
Lines 344 to 346 in 2d02649
On the other hand, we don't bake grapher pages that have been redirected to explorers:
owid-grapher/baker/GrapherBaker.tsx
Lines 377 to 382 in 2d02649
Meanwhile, deleteOldMultiDimPages does delete grapher pages that have been redirected to explorers, and effectively overrides deleteOldGrapher's safeguard. If this safeguard is no longer relevant, I'd remove it rather than having it in one place and not the other, which is confusing.
| console.log(`DELETING ${slug}`) | ||
| const path = `${bakedSiteDir}/grapher/${slug}.html` | ||
| await fs.unlink(path) | ||
| console.log(path) |
There was a problem hiding this comment.
console.log(path) necessary? We have the same console.log in deleteOldGrapher().
Also, instead of:
DELETING coronavirus-cfr
localBake/grapher/coronavirus-cfr.html
we could maybe simply output:
DELETING localBake/grapher/coronavirus-cfr.html
?
| } | ||
| const publishedSlugs = await getAllPublishedMultiDimDataPageSlugs(knex) |
There was a problem hiding this comment.
you could get the list of published slugs from multiDimsBySlug potentially, to avoid the extra db query?
