Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

🧹 Restructure CF functions code into smaller files #4054

Merged
merged 3 commits into from
Oct 28, 2024
Merged

🧹 Restructure CF functions code into smaller files #4054

merged 3 commits into from
Oct 28, 2024

Conversation

danyx23
Copy link
Contributor

@danyx23 danyx23 commented Oct 10, 2024 •
edited
Loading

This PR restructures the by now pretty large grapherRenderer.ts file into a few smaller files that roughly group similar functionality.

@danyx23 danyx23 mentioned this pull request Oct 10, 2024
Closed
This was referenced Oct 10, 2024
Draft
Merged
@danyx23 Graphite App
Copy link
Contributor Author

danyx23 commented Oct 10, 2024 •
edited
Loading

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @danyx23 and the rest of your teammates on Graphite Graphite

@danyx23 danyx23 mentioned this pull request Oct 10, 2024
Merged
@danyx23 danyx23 marked this pull request as ready for review October 10, 2024 13:29
@danyx23 danyx23 changed the base branch from server-side-csv-marcel to graphite-base/4054 October 10, 2024 16:57
@danyx23 danyx23 force-pushed the server-side-csv-file-restructure branch from 305f9a8 to 8a50281 Compare October 10, 2024 16:59
@danyx23 danyx23 changed the base branch from graphite-base/4054 to master October 10, 2024 17:00
@danyx23 danyx23 force-pushed the server-side-csv-file-restructure branch from 8a50281 to 7bf20b8 Compare October 10, 2024 17:00
marcelgerber
marcelgerber approved these changes Oct 10, 2024
View reviewed changes
Copy link
Member

@marcelgerber marcelgerber left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lords work! Thank you 🙏

functions/grapher/by-uuid/[uuid].ts Outdated
Comment on lines 1 to 2
import { Env } from "../../_common/env.js"
import {
extensions,
fetchGrapherConfig,
} from "../../_common/grapherRenderer.js"
import { extensions } from "../../_common/env.js"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Multiple imports of the same file

functions/grapher/[slug].ts Outdated
@@ -1,17 +1,18 @@
import { Env } from "../_common/env.js"
import { Env, extensions } from "../_common/env.js"
import { Etag } from "../_common/grapherRenderer.js"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seeing that this is just a type alias, it could possibly also go into env.ts

functions/_common/metadataTools.ts Outdated

const metadataCols = getColumnsForMetadata(grapher)

const columns: [
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should also extract this (very long!) type definition to someplace else - possibly simply at the top of this file?

Also, VS Code is telling me about a type error here, so maybe double-check that it is actually correct?

functions/_common/fetchMetadataForGrapher.ts Outdated
import { Grapher } from "@ourworldindata/grapher"
import { OwidColumnDef } from "@ourworldindata/types"
import { StatusError } from "itty-router"
import { createZip } from "littlezipper"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
import { createZip } from "littlezipper"
import { createZip, File } from "littlezipper"

@danyx23 danyx23 force-pushed the server-side-csv-file-restructure branch from 7bf20b8 to f585721 Compare October 11, 2024 16:00
@danyx23 danyx23 force-pushed the server-side-csv-file-restructure branch from f585721 to 19e2195 Compare October 24, 2024 15:00
@danyx23 danyx23 force-pushed the server-side-csv-file-restructure branch from 19e2195 to 7220fe7 Compare October 24, 2024 15:23
@danyx23
Copy link
Contributor Author

danyx23 commented Oct 24, 2024

Hi @marcelgerber! I just polished and rebased this - I think it's good to merge from my side. Can you give it a last look to see if you are happy with the changes? There is one changeset with everything since your first review.

ikesau
ikesau approved these changes Oct 28, 2024
View reviewed changes
Copy link
Member

@ikesau ikesau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Always difficult to review these sorts of PRs with high confidence that I caught everything, but I tested each route locally with 2 slugs/uuids each and they all worked (except the human trafficking one) so I think we should be good?

functions/_common/metadataTools.ts Outdated
searchParams: URLSearchParams
) {
const useShortNames = searchParams.get("useColumnShortNames") === "true"
console.log("useShortNames", useShortNames)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. Just an observation: there's lots of logging throughout these files which makes it hard to evaluate when they're meant to be included and when they're debugging leftovers (which is what this one feels like)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, removed this one and another one.

functions/_common/downloadFunctions.ts
return Response.json(fullMetadata)
}

export async function fetchZipForGrapher(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not related to this change as it also happens in master, but in my random testing with URLs in my address bar history, I discovered that http://localhost:8788/grapher/human-trafficking-victims-under-18-years-old-male-vs-female.zip 404s:

Handling error TypeError: Cannot read properties of undefined (reading 'map') at getCitationShort

Should I make a ticket?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A good catch. I fixed this (wrong handling of origins in one place).

@danyx23 danyx23 merged commit d2370af into master Oct 28, 2024
15 of 17 checks passed
@danyx23 danyx23 deleted the server-side-csv-file-restructure branch October 28, 2024 20:10
@ikesau ikesau mentioned this pull request Oct 30, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ikesau ikesau ikesau approved these changes

@marcelgerber marcelgerber Awaiting requested review from marcelgerber

Assignees

@danyx23 danyx23

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@danyx23 @marcelgerber @ikesau