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
enhancement: add support for codemods command to run on plugins #19817
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
packages/utils/upgrade/src/modules/codemod-repository/repository.ts
Outdated
Show resolved
Hide resolved
packages/utils/upgrade/src/modules/project/__tests__/project.test.ts
Outdated
Show resolved
Hide resolved
Co-authored-by: Jean-Sébastien Herbaux <jean-sebastien.herbaux@epitech.eu>
…ry.ts Co-authored-by: Jean-Sébastien Herbaux <jean-sebastien.herbaux@epitech.eu>
Co-authored-by: Jean-Sébastien Herbaux <jean-sebastien.herbaux@epitech.eu>
} from './types'; | ||
|
||
export class Project implements ProjectInterface { | ||
export class Project implements IProjectBase { |
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.
I need to find where we had this discussion (I think it's internal).
But let's keep our distance from the C# interface notation to stay consistent with the rest of the codebase (well, except DTS, but we'll need to worry about that later)
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.
Yeah, the problem is it interferes with the class imports of the same name. Do we have guidelines on that? I think we shouldn't be using classes at all, but I didn't want to refactor the entire thing for this little feature.
@@ -9,7 +9,7 @@ import * as f from '../../modules/format'; | |||
import { Version } from '../../modules/version'; | |||
|
|||
import type { UpgradeOptions } from './types'; | |||
import { PluginProject } from '../../modules/project/project'; | |||
import { isPluginProject } from '../../modules/project/project'; |
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.
Import order? I'm surprised eslint didn't complain about that 🤔
Also, you can shorten the import by removing the last /project
@@ -21,6 +21,33 @@ import type { | |||
IProjectBase, | |||
} from './types'; | |||
|
|||
export const isPluginProject = (project: unknown): project is IPluginProject => { |
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.
What do you think about having a utils
export within project to separate the class/main business logic from the utilities that lives outside?
packages/utils/upgrade/src/modules/project/__tests__/project.test.ts
Outdated
Show resolved
Hide resolved
The project exports in the utils/upgrade module have been restructured. Now, the classes 'Project', 'AppProject', and 'PluginProject' are exported as types.
Simplified the exports in the project module in utils/upgrade module. Instead of exporting each util individually, all the utils under './utils' are now being exported together.
Simplified `isPluginProject`, `assertPluginProject`, `isAppProject`, and `assertAppProject` functions by removing unnecessary conditions. The refactoring allows the functions to focus solely on type verification, reducing the complexity and improving the readability of the code.
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.
QAed, seems to be working well
What does it do?
Allows the codemods argument to run on plugins instead of only strapi projects
Why is it needed?
To help plugin writers migrate their plugins to Strapi 5
How to test it?
codemods
command against a plugin; it should workupgrade
and it should still require being in a Strapi projectRelated issue(s)/PR(s)
Let us know if this is related to any issue/pull request