Code review for types - minimalist file cache

[uneven path]

Mostly looking at the types, is there a better approach to this?

import { Constituency } from "@/server/types";

const TAGS = [
    "shapes",
    "constituency-parties",
    "constituency-to-party-candidates",
];
const fileCache: Record<string, object> = {};

type Tags = typeof TAGS;
type Tag = Tags[number];
type PoliticalParty = string;
type Candidate = {
    name: string,
    twitter: string,
} | {
    name: null,
    twitter: null,
}

interface Candidates {
    conservative: Candidate | null,
    labour: Candidate | null,
    lib_dem: Candidate | null,
    snp: Candidate | null,
}

async function get(name: Tag): Promise<Record<Constituency, unknown>>;
async function get(name: "shapes"): Promise<Record<Constituency, [number, number][]>>;
async function get(name: "constituency-parties"): Promise<Record<Constituency, PoliticalParty>>;
async function get(name: "constituency-to-party-candidates"): Promise<Record<Constituency, Candidates>>;
async function get(name: Tag) {
    const file = Bun.file(`public/json/${name}.json`);
    const payload = await file.json();
    fileCache[name] = payload;

    return payload;
}

(Bun.file can be mentally replaced with fsPromises.read for clarity's sake)

[twin fox]

@uneven path Did you mean to have as const on TAGS? Otherwise Tags is just string[] and Tag is just string

[uneven path]

Oh, no I did not and truthfully I don't understand how that works. I'm guessing it's casting whatever i define as a literal?

[twin fox]

Yeah, it preserves the literal type values, so Tag is "shapes" | "constituency-parties" | "constituency-to-party-candidates" not an arbitrary string.

[uneven path]

Crazy how you caught that, as the auto-complete I wanted to setup when passing name to get() still works.

Consider me fooled by my own code lmao

[twin fox]

Yeah, since you overloaded it you get autocomplete for the specific overloads you defined.

[uneven path]

Ahhh, that makes a lot of sense. Found what you were talking about (sure enough you're right) and corrected it, nice find. 👀

[twin fox]

I don't know if you control the data source here, but I'd probably drop the { name: null, twitter: null } verison of Candidate and just have the object itself be null

(Even if you don't control the data, I guess, that's the sort of transform I'd do when reading the data so that my code has a consistent representation of the data)

[uneven path]

You're a wizard dude.

export type Candidate =
    | {
          name: string;
          twitter: string;
      }
    | {
          name: null;
          twitter: null;
      };

The actual implementation is a bit wrong anyway, I either have the fields as Candidate or null, so the appendage is redundant. I'll finish that now.

Is the type alias PoliticalParty fine? I normally alias my types as a means of naming them as I do my variables for readability.

Ideally I want to keep things simple but also readable and struggling to get that balance, hoping that's alright.

[twin fox]

Aliasing string for readability is okay - I don't find it's a super common convention, so my first instinct would be to expect it to be something specific, like Tag - but I do see the appeal of those sort of aliases

Though if there is a limited set of valid values, you might consider making it a specific type.

Like I don't know if this is accurate to your data, but if it is, you could do:

type PoliticalParty = "conservative" | "labour" | "lib_dem" | "snp";
type Candidates = Record<PoliticalParty, Candidate | null>;

[uneven path]

I like that actually, and yes it's only those specific keys and no others, so narrowing that type would definitely help.

[twin fox]

(If being able to loop over the parties is useful, then doing the pattern with TAGS where you define an as const array and then get the type from it is something you could do, too)

[uneven path]

I do not loop over the parties at the moment, I only reference them, though I did consider doing that just so I don't have to write out the political parties again.

Then I remembered Ctrl+C/V exists.

[twin fox]

The other reason you might end up needing looping is beacuse looping over object keys in TS can be annoying: TS won't assume that objects don't have excess keys.

So it's actually easier to do:

const candidates: Candidates;

for(const party of PARTIES) {
    // party is PoliticalParty, this works
    doSomething(candidates[party]);
}

than it is to loop the object directly with something like:

for(const party of Object.keys(candidates)) {
    // party is string, this doesn't work
    doSomething(candidates[party]);
}

[uneven path]

In retrospect I actually don't need TAGS, as originally I used that as a poor man's enum when the source was JS to avoid errors. Since I migrated to TS, this auto-complete shouldn't warrant that anymore.

I still like the idea of as const though and will definitely keep that in mind for cases where I may want an array value AND type deriving from.

[uneven path]

What about entries? Is that handled fine?

[twin fox]

entries?

Oh, you mean Object.entries? No, same issue as Object.keys, I'm pretty sure

Well, yes and no.

[uneven path]

I'm definitely getting the types when writing out a for loop in the file, unsure where what you mentioned applies.

It also works in other files too. 🤔

Maybe it's a difference in configuration?

[twin fox]

It sort of works:

[rare wing]

for(const [
  party,
//^? - const party: string
  value
//^? - const value: Candidate | null
] of Object.entries(candidates)) {}

[twin fox]

party there is string, even though it really should be PoliticalParty.

But you may not need party to be specifically typed since you've already got access to value. (Though you might still need the correct typing for party depending on what you're doing)

Technically it really shouldn't work this much - value being typed as Candidate | null is actually kind of a soundness bug 😅

[uneven path]

That's fair. I'll keep that in mind moving forward then if the types look or feel awkward.

Hopefully that'll be fixed so that it gives the actual type? Sounds like a resolution bug lowkey.

[twin fox]

No, the existing behavior of Object.keys is the correct behavior.

!:unsafe-keys-example

[rare wing]

retsam19#0

`!retsam19:unsafe-keys-example`:

An example where assuming Object.keys(x) returns keyof (typeof x) is unsafe:

type XY = {
   x: string,
   y: string,
}
function louder(obj: XY) {
    Object.keys(obj).forEach(key => {
        // If key is "x" | "y", then this compiles:
        console.log(obj[key].toUpperCase());
    })
}
const obj = { x: "x", y: "y", zero: 0};
louder(obj); // compiles, because extra properties are allowed... but crashes at runtime

[twin fox]

TS allows objects to have more properties than specified in their types - it's a fairly useful part of the type-system - but that means that you can't necessarily assume that an object only has the properties that you expect.

[uneven path]

Oh I see, are there any rules that can prevent this out of curiosity?

[twin fox]

(Basically TS treats every type more like a Java interface)

[uneven path]

Totally get that.

[twin fox]

Not really, no. There's persistent talk of having some sort of Exact<Type> utility that could say "this type has these properties and no others", but it's tricky because it breaks a lot of the rules that the type system otherwise operates on about what's allowed.

For the most part, it's not a huge deal. You can often structure things in a way that makes it not an issue, and if you need to, I generally think it's fine to use "unsafe" tools for asssuming that an object doesn't have extra keys if you know better, e.g.

!*:unsafe-keys

[rare wing]

retsam19#0

`!retsam19:unsafe-keys`:

Since TS allows objects to have extra properties not specified in the type, it doesn't assume that all the keys on the type are the only keys on the object. This means that Object.keys returns string[] not a specific type, and for(const key in obj), key is string, (not keyof typeof obj).

If you wish to assume otherwise, this utility is often helpful:

// A signature for `Object.keys` that assumes the only keys are the ones indicated by the type
const unsafeKeys = Object.keys as <T>(obj: T) => Array<keyof T>;

[uneven path]

I would definitely prefer to keep it strict where possible, so maybe I'll leave unsafe keys for another time where I'm just sick of everything and tight on time lol

[twin fox]

Yeah, that's why I suggested looping the array and using that to key the object as a safe alternative; but yeah, you may not need it.

[uneven path]

Oh I see, I'll definitely keep that in mind moving forward then. :)

Is there anything else that could be changed from a general perspective or is this fine?

[twin fox]

Yeah, one last thing - if you turn resolveJsonModules on you can import .json files and TS will infer types for them. So you could theoretically do something like:

async function getShapes() {
  return await import("public/json/shapes.json");
}

and avoid all the overloading/tag stuff on get

(And if this is a backend service, I'm not sure I'd even bother doing a dynamic import compared to just importing the JSON with a normal import)

Downside is that TS won't infer fully specific types for JSON files that it imports: there's not yet any as const equivalent for importing json.

[uneven path]

I'm not too sure how much faster that is compared to a simple read but I know where you're getting at, modules when imported are cached, right? Meaning a lot of what I've written could be redundant.

[twin fox]

Yeah, I'm not 100% if dynamic import caches out of the box, but regular top-level imports definitely do.

[uneven path]

Thoughts on await import("...") as Type?

[uneven path]

They basically do in every language I've used thus far, and doing a tiny bit of research yields they do the same in Node (will have to ask if they do it in Bun, I'm hoping so).

[twin fox]

Yeah, they probably do; just not something I use a lot of in node. (More often in a front-end bundled application)

[twin fox]

It's not safe, but with resolveJsonModules you'd probably still get an error if they were too different.

The other possible option; sometimes it just makes sense to move the .json into a .ts file with a default export. A bit silly, but you can get full type-checking that way.

[uneven path]

That might make me pass in my sleep, I couldn't imagine doing that lol

[twin fox]

I've done far worse.

[uneven path]

The reason why I've kept them as JSON is actually because there's SO MUCH data that a real solution would really be a database. The data also sometimes changes (purely additions), so it wouldn't make sense/be worth the effort (IMO) to retrieve data -> convert to JSON -> convert to TS.

The whole reason I have these types in place is for temporary sanity and clarity.

[twin fox]

"Convert to TS" is basically nothing since JSON is a syntax subset of TS, the file really can be export default { /* JSON BLOB */ } as const

But yeah, could be an extra step which might not be worth it just for a bit of extra type-safety.

[uneven path]

Good point in retrospect, another concern is if the type-checker would seize up if I were to do that.

[twin fox]

Yeah, might depend on the file size. It can handle a lot - like the object that has every localization string used in our codebase - but if we're talking MBs of data maybe not great.

Another option for JSON files is I'm pretty sure you can technically put a foo.d.ts file adjacent to a foo.json file and TS will use the .d.ts typings when the .json is imported. ... but realistically not sure that's better than just import and cast

[uneven path]

Side question because the base description didn't make sense - .d.ts are what, type stubs?

[twin fox]

They're declarations for types.

The main thing they're used for is that when you compile TS files, the compiler outputs the JS file and the .d.ts file that describes the typings.

When you consume a library written in TS, the compiler just reads the declaratoins, not the entire library's source code.

There are some (fairly niche) cases where you do want to hand write them, e.g. but mostly they're a compiler artifact.

[uneven path]

I see, that makes sense. Love that.

I did try to rely on TS reading the JSON files, but I think they're too chunky as they're reporting any now.

Like you said there's no option to define the type directly in the import, so the next best move is to effectively keep the overloads and dynamically import for the types?

Just clarifying incase I misunderstood you earlier.

[uneven path]

Okay, did a bit of thinking for myself as you did a lot of explaining for me which greatly appreciated.

[twin fox]

The .d.ts next to .json might allow you to control the types of normal imports, but yeah, the dynamic imports + casting/overloading is fine, too

[uneven path]

Yeah, gave it a try and TS didn't complain, even got to learn about overwriting types (not needed but helpful).

Went with this approach at the moment, where the imports lie in the files that need them:

And I won't lie, performance TANKED

That's the cold start, the ABSOLUTE WORST I've been which used file I/O was only 10s.

The cache obviously helped reduce this significantly.

What's even more weird about this is that performance AFTERWARDS is actually faster compared to my file I/O, like shockingly fast.

I'll need to do some more looking into, it's possible my dorky overload method might just be all I need until I can shove all of this into a database, so it feels a bit in vain. 😭

Nonetheless I've learned so much about this and I love the tips you gave, thank you so much for your help1 💞