Skip to content

Conversation

@david-crespo
Copy link
Collaborator

This is basically it:

-if (typeof v === "string" && (k?.startsWith("time_") || k === "timestamp")) {
+if (typeof v === "string" && isoDateRegex.test(v)) {

Before this PR, we require a key with a prefix of time_ or an exact match to timestamp in order for us to attempt parsing the value a date. This is pretty bad because there is no reason to expect timestamp properties to follow this pattern — it only happened to be true in omicron for a long time. This was already a problem for other apps using this generator that did not follow this very implicit (secret, even) convention, as @augustuswm can attest.

In this PR, instead of looking at the keys, we just parse as a date anything that matches a regex. This is still suboptimal, because it's easy to imagine a value happens to look like a date but which should be left as a string. For example, imagine setting a date as a description on a resource. The description is intrinsically a string and should be left as one, but this approach will cause it to be parsed as a date, even though the generated types will call it a string. This could obviously cause problems in an app.

The difficulty here is that we don't have the OpenAPI schema on hand at runtime. #279 attempts to solve this more robustly by generating code to parse as a date any fields that say format: date-time in the OpenAPI schema. However, #279 feels so complicated that I would prefer to find a compromise solution that looks more like this PR.

@david-crespo david-crespo changed the title Parse anything that looks like a date Option 1: Parse anything that looks like a date Feb 7, 2025
@augustuswm
Copy link
Contributor

Personally I think despite the calls being made in each operation, Option 2 is the better one as it prioritizes correctness. Even it is unlikely, I don't think we should opt in to potential type incorrectness.

@david-crespo
Copy link
Collaborator Author

Went with #281 for now.

@david-crespo david-crespo deleted the parse-all-dates branch February 7, 2025 16:32
@david-crespo
Copy link
Collaborator Author

Personally I think despite the calls being made in each operation, Option 2 is the better one as it prioritizes correctness. Even it is unlikely, I don't think we should opt in to potential type incorrectness.

I 100% intend to do it the right way soon, just need to unblock myself today.

@david-crespo
Copy link
Collaborator Author

david-crespo commented Feb 7, 2025

More specifically: using the already existing and high-quality generated Zod parsers to parse the responses seems like the obvious thing to do instead of generating this horrible bespoke code. I just need to find a way to do that that I'm happy with.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants