Skip to content
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

fix: save generator after migration #400

Merged
merged 3 commits into from
Jan 14, 2025

Conversation

cristianoventura
Copy link
Collaborator

Description

This PR fixes an issue where the UI detects changes in the generator if it has been migrated during runtime.

How to Test

  • Manually set the version property of a generator to 0
  • Open the generator in the UI
  • Check that no changes are detected
  • Check that the file has been migrated to version: 1.0

Checklist

  • I have performed a self-review of my code.
  • I have added tests for my changes.
  • I have run linter locally (npm run lint) and all checks pass.
  • I have run tests locally (npm test) and all tests pass.
  • I have commented on my code, particularly in hard-to-understand areas.

Screenshots (if appropriate):

Screen.Recording.2025-01-09.at.12.27.46.PM.mov

Related PR(s)/Issue(s)

Resolves #399

@cristianoventura cristianoventura self-assigned this Jan 9, 2025
@cristianoventura cristianoventura requested a review from a team as a code owner January 9, 2025 20:34
@cristianoventura cristianoventura changed the title fix: save generator migration to disk fix: save generator after migration Jan 9, 2025
going-confetti
going-confetti previously approved these changes Jan 10, 2025
generatorStateData: Partial<GeneratorFileData>,
generatorFileData: Partial<GeneratorFileData>
) => {
// Convert to JSON instead of doing deep equal to remove
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a question not related to this particular PR:
I think it shouldn't be a problem here, but since JSON.stringify does not guarantee the order of keys, can two equal objects be considered unequal?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's possible. We might want to switch to isEqual if this proves to be a problem.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC, I've used JSON.stringify because generatorFileData would include all the undefined values, but stored JSON wouldn't have them, and if would show up as false dirty. So we'd need to do some cleanup if we switch to isEqual

Copy link
Collaborator

@e-fisher e-fisher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works like a charm! 🙌 Though, I'm a bit worried about the "hidden" write operation when opening a generator. If we mess up a migration, could this lead to broken generators? Without this change it would still break them on save, but I imagine that would be less impactful, because failing to open a generator I would try others, corrupting them as well.

As an alternative to consider: could we migrate in memory while doing the isDirty check, without writing anything to the file? And only save new migrated version to file after user modified and saved generator.

@Llandy3d
Copy link
Member

Works like a charm! 🙌 Though, I'm a bit worried about the "hidden" write operation when opening a generator. If we mess up a migration, could this lead to broken generators? Without this change it would still break them on save, but I imagine that would be less impactful, because failing to open a generator I would try others, corrupting them as well.

As an alternative to consider: could we migrate in memory while doing the isDirty check, without writing anything to the file? And only save new migrated version to file after user modified and saved generator.

This sounds like a reasonable and neat solution 👀
I wonder if we should also warn that the generator needs to be updated and possibly ask if checking and updating all of them is wanted, this last bit could possibly be something for the future

Copy link
Collaborator Author

@cristianoventura cristianoventura left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've removed the hidden write operation and let Zod to parse the state internally.

Comment on lines +25 to +29
options: generator.options,
testData: generator.testData,
rules: generator.rules,
allowlist: generator.allowlist,
includeStaticAssets: generator.includeStaticAssets,
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue was actually related to the order of the fields in the migration, which caused the JSON.stringify comparison to return false. I made a ticket to address this behaviour #403

Copy link
Collaborator

@e-fisher e-fisher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🚀

@cristianoventura cristianoventura merged commit a1c6d02 into main Jan 14, 2025
3 checks passed
@cristianoventura cristianoventura deleted the fix/generator-unsaved-changes branch January 14, 2025 17:07
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.

Generator prompts for saving when no changes were made
4 participants