-
Notifications
You must be signed in to change notification settings - Fork 2
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.
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
feat: generator schema migration #373
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Migration logic works! 💯
One thing though. The following files are all a part of the schema so they should be moved to the v1
folder:
src/schemas/rules.ts
src/schemas/testData.ts
src/schemas/testOptions.ts
And the instructions should be to copy the entire schema in the v1
folder and rename it v2
. If you don't copy, then you run the risk of changing previous versions of the schema e.g. by making changes to the TestOptionsSchema
.
@allansson Nice catch, I've missed those. I've now updated the schemas + the readme. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with a minor remark.
Apologies for changes requested
- I only chose it because we recently merged an update to ESLint config and I wanted to make sure the branch is updated before merging.
|
||
export type GeneratorSchema = z.infer<typeof GeneratorFileDataSchema> | ||
|
||
// TODO: Migrate generator to the next version |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
v1
is essentially the same as v0
here, right? I don't understand what do we need the TODO comment for - is it for future non-breaking changes to v1
? If so, I'd prefer a regular descriptive comment, so that TODOs are reserved for debt
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They are the same schema, but the implementation of the migrate
function is incomplete because there is nothing to migrate to since v1
is the latest version.
The TODO
comment here indicates that the implementation needs to change when another version exists (to migrate from v1
to v2
).
For when a new migration is needed: | ||
|
||
1. Create a directory for the new version (e.g. `/v2`). | ||
2. Copy files from the previous schema into this directory and make appropriate changes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While out of scope for this PR, I think we should explore autogenerating new schema versions in future - instead of manually creating a copy of the vN
folder and its content, we'd be able to run something like whatever-code-gen-is-trendy-rn studio:generator
and have it do it automatically.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can explore that separately and probably do the same for settings too! 👍
…or-schema-migration
Description
This PR adds migrations to the generator schema, ensuring that changes to its structure remain backward-compatible. This allows the generator to work properly even when the schema is updated.
The pattern adopted in this PR is based on the settings migration #371.
How to Test
/schemas/generator/v1/index.ts
such as renaming a property.migrate
function in/schemas/generator/v0/index.ts
to return the new schema.version: '0'
and all old properties before testing.Checklist
npm run lint
) and all checks pass.npm test
) and all tests pass.Screenshots (if appropriate):
Related PR(s)/Issue(s)
Resolves #370