refactor: Generator parts from interfaces into zod objects #42
Conversation
| export const GeneratorFileData = z.object({ | ||
| name: z.string(), | ||
| version: z.string(), | ||
| recordingPath: z.string(), | ||
| options: TestOptions, | ||
| testData: TestData, | ||
| rules: TestRule.array(), | ||
| allowlist: z.string().array(), | ||
| }) | ||
|
|
||
| export type GeneratorFileData = z.infer<typeof GeneratorFileData> |
There was a problem hiding this comment.
I know there are no conflicts between exported type and const of the same name, but I would still prefer zod objects to be appended by Schema to be more explicit:
export const GeneratorFileDataSchema = z.object({ .. })
export type GeneratorFileData = z.infer<typeof GeneratorFileData>WDYT?
There was a problem hiding this comment.
I think I like that the names are shared so depending on where you use it you get the right "object" 🤔
In theory you know that it's a schema because it will be imported from src/schemas/... and inspecting the object also clearly gives it away.
I find it really nice and reduces repetition/verbosity.
It's also true that I come from Python where it's done the same way, you use the object both as the object to construct and as a type, so maybe for typescript let's wait for more feedback before taking a decision 💭
There was a problem hiding this comment.
I think it's a bit magical and I was surprised to learn TypeScript doesn't throw an error in such cases.
Another downside of using the same names is that it's harder to understand what's being imported when you look at the import statements (it's probably a nitpick though)
I think I agree with Edgar that we should append Schema to the schema consts names.
I also suggest we keep the types in src/types - for example, in src/types/rules.ts you'd just import the schema and export the inferred type like this:
import { TestRuleSchema } from '@schemas/rules.ts'
export type TestRuleSchema = z.infer<typeof TestRuleSchema>This way we'll end up keeping schemas and types in separate folders and will be importing types from src/types (among other benefits, schema files will only schema files only have schema definitions in them, which is minor, but I think it'll improve readability)
There was a problem hiding this comment.
I think it makes sense, once you "compile" the code types are stripped out so if you are importing and using something it's the actual object.
I have a preference for that approach probably because I'm coming from python and it's less verbose but if the majority of the team thinks that separating schemas & types aids with readability, I think it's worth the cost of a few more lines 🙌
| // TODO: we might want to send an error on wrong file, a system to send and show errors from the main process | ||
| // could be levereged for many things mmmm |
There was a problem hiding this comment.
Either messaging between render and main OR do the parsing on render side and let it handle there 🤔
There was a problem hiding this comment.
oh, I left the comment there! 😄
Regardless of leveraging it in here or not, that system would be useful for having a channel between main/renderer to just send errors on any operations, the item has already been added on the todo list.
In the future we might move more operations on the main process, so handling the parsing on the renderer side would just be probably a temporary solution 🤔
This reverts commit e82b4bb.
Llandy3d
force-pushed
the
load_generator_file
branch
from
2e325e7
to
8e36198
Compare
The refactoring was done to have validation for loading and possibly also for saving the file 🤔
I opted to have all the inferring together at the end of the file, if you prefer another approach feel free to raise it ( I just think it looks more readable this way)
Feel free to ignore the loading generator file logic, that will be completed in a separate PR
This is part of https://github.com/grafana/k6-cloud/issues/2520