[xc-admin-frontend] Unify publisher keys file schema with observer and balance tracker

[mcamou]

Deployment of this in the k8s node needs to be coordinated with observer, balance tracker and the deduplication of key files in the deployment repo.

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
xc-admin-frontend	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Jun 18, 2024 5:53pm

[cprussin]

Minor suggestions here, let me know if you need any specific help with any of these!

[cprussin]

I would use the promisified version of this instead of using the sync version, the sync version is blocking and using the promisified version and turning this into an async function will enable you to use Promise.all to do the io in parallel

[mcamou]

yeah I thought of that but since it's only done once during startup and these are fairly small files (<30K) I thought it would be clearer to just take the minimal I/O hit.

[cprussin]

In general with modern node, there's not much of a good reason to prefer sync over async. It's semantically incorrect and doesn't produce any better ergonomics since you could very easily use the async version here and just add await on line 34 and 35 and achieve pretty much the exact ergonomics you have here without the semantic incorrectness (it would still be blocking but at least it would be blocking in an obvious way at the callsite).

I don't know any good reason to use the sync functions nowadays.

[cprussin]

Oh actually I just remembered that exists has been deprecated in node, one of the reasons is that doing this causes race conditions (obviously not in this case, but in the general case, a file could be removed between the call to exists and the call to readFile).

The more "correct" way to do this is to use readFile in a try-catch and handle ENOENT as a "file does not exist" error

[cprussin]

Note that the type casting here removes typescript's ability to actually guarantee anything about the types. IMO this shouldn't even exist in typescript.

Typically any time I load data from an external source I use zod to parse the data so I can guarantee that my typescript types are actually correct and can handle invalid data accordingly, rather than typecasting and assuming the types are accurate.

[mcamou]

yeah I know... the original code didn't have any typechecking either so I put in the cast to shut the compiler up. Since this is a file that we control (and that at least 2 other projects will use) I think it's fairly safe to do this, WDYT?

[cprussin]

Doing zod here is pretty trivial:

import { z } from 'zod';

const schema = z.array(z.strictObject({
    key: z.string(),
    name: z.string()
}));

...

const data = await schema.parseAsync(YAML.parse(await readFile(filename, 'utf8')));
return Object.fromEntries(data.map(({ key, name }) => [key, name]));

Not using any kind of schema parsing and just type casting is a bit analogous to using an unsafe block in Rust and not using something like serde to load data. There are very very few good reasons to do that, mostly heavy performance optimizations in very hot codepaths with very tightly controlled external data sources.

If the ergonomics of adding proper parsing were significant then I'd feel less strongly on it, but it's arguably clearer and easier to type out than even writing the type cast since you're literally just writing the shape of the data. Plus, while I think this case is fairly safe, things could easily get refactored or skewed over time and having this in place would make it drastically easier to chase down if something was amiss.

So tl;dr yes I still recommend it even in "safe" places such as this

[cprussin]

as a side note I also think the amount of things people do in typescript to silence the compiler without actually fixing the underlying type safety issues is a huge problem and it's a big reason why typescript isn't nearly as reliable as something like rust.... good discipline pays off IMHO.

[mcamou]

I could be persuaded to add some rudimentary validation and throwing exceptions (or os.exit(1)) on failure.

[cprussin]

You could do that, but it will be more work (at least to do this to the extent where you don't need type casting) and less flexible than just using zod. We need zod for this app anyways so it wouldn't be like it's a dependency I don't plan to introduce regardless.

[mcamou]

There you go @cprussin. Async, Zod, unicorns and angels :) Thanks for the review!