-
Notifications
You must be signed in to change notification settings - Fork 8
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
Add a codemod for transforming configs to the new format #86
Conversation
@daniilsapa what do you think? |
Overall impressionWell done! This solution is extremely graceful and simple for a user, and yeah, a huge command should not be a problem at all as long as you can copy and paste it 😄 This is a very unexpected solution. I couldn't even imagine something like this. I never came across jscodeshift library and generally such a type of migration scripts/tasks, so it was my blind spot. I took some ideas into account. Thank you for that! QuestionsI have some questions that could potentially improve the solution
Additional actionsWe may need additional action items that would make the migration process smoother. Should we create additional issues for them or address them as part of the current one?
|
.filter((path) => ruleNames.includes(path.node.value)) | ||
.replaceWith((path) => j.stringLiteral(`fsd/${path.node.value}`)) | ||
|
||
// Make the default export or the argument of `defineConfig` an array |
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.
Shouldn’t we also add an import for the fsd plugin and that …fsd.plugins.recommended
line in the config definition? Because if we run the current migration, the config will have only the rules that were in the initial config. And since the main feature of the previous config was just to allow you to entirely disable a rule, the user most likely ends up with a config with all rules disabled and without rule definitions provided to Steiger.
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.
ah, yeah, good catch! we do need to add that
.find(j.Literal) | ||
.filter((path) => ruleNames.includes(path.node.value)) | ||
.replaceWith((path) => j.stringLiteral(`fsd/${path.node.value}`)) | ||
|
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.
Optional. We could check if the config is already in the new shape, just in case. But it seems like that would be a very rare edge case.
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.
If it already is, then it doesn't make sense for the user to run the migration, I think they'll know better. So let's just assume that there is something worth migrating
Yes, I think that makes sense. We can add the message to the config validator
No, it was just an example. You can also do
Yes, let's do that
Yeah, I agree that it would be nice. I'm not sure if it's best to have a descriptive name for this migration or simply call it by the version that it's migrating from (e.g.
I think we can make them in separate issues. The linter is currently not in a release-able state anyway, so it's fine to do it incrementally, I think |
@daniilsapa adjusted the codemod to also add the |
@illright |
@illright |
Yeah, I agree, I also thought that we should've developed in a branch. Since we have a bunch of unreleased work already, I think it's fine to keep merging to master, and we'll keep it in mind for future releases |
Closes #75
Create a
steiger.config.js
in the current directory and test the codemod with the following command:This command is huge, but the migration is so simple that I don't think our users will mind. I suggest writing this in the migration guide, although we will replace the branch name once this is merged.
Alternatively, you can test it here in the browser by running this in the terminal of StackBlitz: