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

[FEAT] Add option to delete stale files #232

Open
lensbart opened this issue Feb 13, 2024 · 4 comments
Open

[FEAT] Add option to delete stale files #232

lensbart opened this issue Feb 13, 2024 · 4 comments

Comments

@lensbart
Copy link

lensbart commented Feb 13, 2024

Is your feature request related to a problem? Please describe.
When you’re making a larger change, sometimes many files can be renamed or deleted and created at once. In such a situation, it could be helpful if this plugin would delete all files it doesn’t recognise in the output folders (that is, all files excluding .graphql files, mapper files, and files that would be generated if missing).

Describe the solution you'd like
This should probably not be the default behaviour, as it is a destructive action that will require the developer to keep unrelated files (for e.g. helper functions) out of the resolvers folders. So I’m thinking of a configuration option

Describe alternatives you've considered

  • Checking the git deletions and creations before committing (status quo, brittle)
  • Enabling the import/no-unused-modules plugin. This might be a good and sufficient approach, but requires a bit more work to set up, makes assumptions about the tooling (some project might be using Biome or might not be using this plugin, not sure what the performance implications are)

Additional context
Not an urgent request, just a friendly suggestion 😊 Thanks

@eddeee888
Copy link
Owner

eddeee888 commented Feb 19, 2024

Thank you for the suggestion! I think there are a few cases we'd have to consider so we don't do destructive actions too aggressively:

  • If the user just renames types, would they expect the codegen to (a) delete the file or (b) marks the file as no longer in the Graph but also create a new file so user can manually migrate
  • If the user wants to extract logic from resolver files or add extra files into resolvers folder, do we need different config to avoid deleting these? e.g.
book
  -- resolvers
        -- Query 
              -- readBook.ts // Query.readBook resolver, calls markBookAsViewed
              -- markBook.ts // Query.markBook resolver, calls markBookAsViewed
              -- markBookAsViewed.ts // utility function to call a backend to mark a book as viewed
  • (Probably a lot more cases I'm not thinking of here)

@lensbart
Copy link
Author

lensbart commented Feb 24, 2024

Good questions. Not sure if my suggestion is worth the potential hassle (especially considering the minor benefit).

If the user just renames types, would they expect the codegen to (a) delete the file or (b) marks the file as no longer in the Graph but also create a new file so user can manually migrate

I’d expect an incremental migration path to first have both resolvers in the schema, and then only the newer. Once a field gets removed from the schema, I think it can realistically be considered as having been migrated. There is always the git history to retrieve old logic.

If the user wants to extract logic from resolver files or add extra files into resolvers folder, do we need different config to avoid deleting these? e.g.

Good question. The more I think of it, the more I’m hesitant about the above suggestion. Even if opt-in via a configuration option, this will introduce a convention in the codebase where files might get deleted unexpectedly. One approach could be to prepend a comment to the generated files, and only auto-delete files that start with that comment.

@eddeee888
Copy link
Owner

All great thoughts here @lensbart! It seems like the emerging theme is "somehow detect and track a file generated by this preset".
Another option is we track the resolver files with something like .resolvers.ts e.g. User.resolvers.ts, Query/deleteUser.resolvers.ts

This way, it has a few benefits:

  • We can detect .resolvers.ts files that are not in the schema
  • We can add FIXME at the top of files that are not in the schema so users can delete them manually... Or be agressive and delete them with a config option (could be a bit hard as codegen preset doesn't support deletion, I think)

This is also a breaking change to existing codebases so we need a migration path e.g.

  • Once implemented, mark it as experimental and disabled by default, until v1
  • Until v1, have a warning telling users that it's going to be enabled by default
  • Maybe the preset can help rename (or mark the files to be renamed) files automatically?

Also, this is not the only option, just another one to add to your great suggestions above 🙂 So I'm keen to have more ideas before picking one

@laytong
Copy link

laytong commented Oct 4, 2024

I also think there's a place for a warning/linting style approach?

i.e. produce a report of potentially no longer required/unused resolvers. detect resolvers that don't match a schema definition.

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

No branches or pull requests

3 participants