Refactor Plugin Class and Introduce New Plugins Command #7839
Conversation
| : component.filesystem.byRegex(pluginDef.pattern); | ||
| } | ||
|
|
||
| private static isImportedComponent(component: Component): boolean { |
There was a problem hiding this comment.
Why is it important that it's imported it not?
And how this actually check if it is imported?
We shouldn't have any diff between imoorted or authored components.
There was a problem hiding this comment.
If the files are absolute path, then it is imported
The thing is that sometimes it would not load an installed env for some reason at first attempt. Not sure why. I will try to look into it.
There was a problem hiding this comment.
ok now I understand
If we have an env installed in the workspace but not set to any component (even with pluginFile) then it will fail because it will not find it in the roots
There was a problem hiding this comment.
anyway I removed this check it's redundant, The problem was that the envs source files in the env's package (node_modules) were corrupted so I had to direct the path to the dist
| return path.isAbsolute(component.filesystem.files[0]?.path || ''); | ||
| } | ||
|
|
||
| private static constructWarningMessage(component: Component): string | undefined { |
There was a problem hiding this comment.
This function name doesn't imply what type of warning it is construct.
There was a problem hiding this comment.
Yeah, at the beginning, I was constructing two kinds of messages - for installed envs and local, but then I realized that the installed message is wrong since it doesn't load it by pattern only at the bringing (it doesn't have the files for some reason)
| private static constructWarningMessage(component: Component): string | undefined { | ||
| if (this.isImportedComponent(component)) { | ||
| return ( | ||
| `env with id: ${chalk.blue(component.id.toString())} could not be loaded.\n` + |
There was a problem hiding this comment.
You don't need that + between lines with \n
Just put all in one string literal
| getPluginDefs() { | ||
| this.getPluginDefsPatterns(); |
There was a problem hiding this comment.
Why do you need that line here? You are not using the result
There was a problem hiding this comment.
sorry leftovers from some testing I was doing
| const files = this.getFileMatches(component, pluginDef); | ||
|
|
||
| if (files.length === 0 && !Plugins.checkedComponents.has(component.id.toString())) { | ||
| logger.consoleWarning(this.constructWarningMessage(component)); |
There was a problem hiding this comment.
Most components don't have plugins inside.
Also many aspects are not plugins.
So we need to make sure we don't show that error when not necessary
There was a problem hiding this comment.
not sure how a regular component or an aspect will get to this check
| @@ -21,6 +23,7 @@ export class Plugins { | |||
| // return plugin.def.dependencies; | |||
| // }); | |||
| // } | |||
| private static checkedComponents: Set<string> = new Set(); | |||
There was a problem hiding this comment.
This is not really checked components but non plugin components.
You only add components that are not plugins. Not any component you visited.
Description:
This PR introduces several significant changes:
Plugin Class Refactor:
Pluginclass has been refactored to improve error messaging. An actionable error message is now displayed when no matching patterns are found for an env component.Caching Mechanism:
New
PluginsCommand:pluginshas been introduced.--patternsflag, which displays all available patterns that plugins can utilize.In addition to the above, the PR includes standard code quality improvements and enhanced modularity for future maintainability.
Feedback and reviews are appreciated.