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

Refactor Plugin Class and Introduce New Plugins Command #7839

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

NitsanCohen770
Copy link
Contributor

@NitsanCohen770 NitsanCohen770 commented Aug 27, 2023

Description:

This PR introduces several significant changes:

  1. Plugin Class Refactor:

    • The Plugin class has been refactored to improve error messaging. An actionable error message is now displayed when no matching patterns are found for an env component.
    • The logic has been optimized with caching mechanisms to ensure that the plugin doesn't attempt to reload a component if it failed to load once, preventing unnecessary repetitions and enhancing performance.
  2. Caching Mechanism:

    • Introduced caching for plugin components to enhance performance and reduce the overhead of repetitive tasks.
    • Leveraged caching to prevent the display of redundant warning messages, providing a cleaner user experience.
  3. New Plugins Command:

    • A new command plugins has been introduced.
    • This decision was taken as plugins are not always defined as environments. For instance, in scenarios like apps. Using a dedicated command makes the functionality more intuitive and user-friendly.
    • Currently, the command supports the --patterns flag, which displays all available patterns that plugins can utilize.
    • The vision for this feature is to introduce more flags and display additional information about the plugins, making it a comprehensive tool for users to manage and retrieve details about their plugins.

In addition to the above, the PR includes standard code quality improvements and enhanced modularity for future maintainability.

Feedback and reviews are appreciated.

: component.filesystem.byRegex(pluginDef.pattern);
}

private static isImportedComponent(component: Component): boolean {
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@NitsanCohen770 NitsanCohen770 Aug 27, 2023

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 {
Copy link
Member

Choose a reason for hiding this comment

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

This function name doesn't imply what type of warning it is construct.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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` +
Copy link
Member

Choose a reason for hiding this comment

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

You don't need that + between lines with \n
Just put all in one string literal

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

getPluginDefs() {
this.getPluginDefsPatterns();
Copy link
Member

Choose a reason for hiding this comment

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

Why do you need that line here? You are not using the result

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sorry leftovers from some testing I was doing

scopes/envs/envs/environments.main.runtime.ts Outdated Show resolved Hide resolved
const files = this.getFileMatches(component, pluginDef);

if (files.length === 0 && !Plugins.checkedComponents.has(component.id.toString())) {
logger.consoleWarning(this.constructWarningMessage(component));
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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();
Copy link
Member

Choose a reason for hiding this comment

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

This is not really checked components but non plugin components.
You only add components that are not plugins. Not any component you visited.

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.

2 participants