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

Allow user to get plugin config from client #1133

Merged

Conversation

pileks
Copy link
Contributor

@pileks pileks commented Aug 11, 2022

Closes #839
The implementation fetches the plugin config by calling its factory function within the PluginPackage and extracting the config from the resulting object.

@krisbitney
Copy link
Contributor

It seems expensive to construct a plugin instance to get the config. What are the pros and cons of changing the PluginPackage type to include a pointer to the config?

export type PluginPackage<TConfig> = {
  factory: () => PluginModule<TConfig>;
  manifest: PluginPackageManifest;
  config: TConfig;
};

@pileks
Copy link
Contributor Author

pileks commented Aug 16, 2022

@krisbitney The main con that came to mind when looking at this issue was that we would need to trust the developer that their implementation of the PluginFactory for their plugin actually assigns the config being used by the plugin to that field (or assigns it at all).
This does sound like something we could enforce though, so it could be considered a moot point. 😄

My main issue in general here has to do with the recent work of moving away from plugin configs and placing any configuration into Env. I have actually recently worked on an issue that's related to this: #1060

The question of "how much time we want to spend on this?" kind of comes to mind, and whether we are really moving away from configs into envs, making even my implementation obsolete at one point in the future. And in any case, do we foresee that we'll be using this getConfig often enough to warrant us making these significant changes in order to be able to fetch the config of a plugin.

I'm sure that I'm missing something important when considering all of this and I'd love to chat regarding this so feel free to hit me up on Discord and we'll come up with something smart!

@krisbitney
Copy link
Contributor

All of that makes sense. Thanks!

@Tracer.traceMethod("PolywrapClient: getPluginConfig")
public async getPluginConfig<TUri extends Uri | string>(
uri: TUri,
options: GetPluginConfigOptions = {}
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we are using options to interact with the getPlugin function, I think it would make sense to re-use the already existing type GetPluginOptions

@nerfZael
Copy link
Contributor

I don't think this feature makes sense, since we are trying to phase out plugins from the client
Even if it did, why not do client.getPlugin() then get the config from there?

On your point of moving from configs to envs, that's not completely true, configs can do what envs can not (e.g. pass objects by reference) so they will always have their place.

@dOrgJelli
Copy link
Contributor

I agree with Jure, I think we should instead just expose a helper client.getPlugin(uri) method.

Copy link
Contributor

@dOrgJelli dOrgJelli left a comment

Choose a reason for hiding this comment

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

comment above

@pileks pileks marked this pull request as draft September 8, 2022 15:36
@pileks pileks force-pushed the pileks-allow-user-to-get-plugin-config-from-client branch from 900e6e0 to 20381d2 Compare September 8, 2022 15:55
@pileks pileks self-assigned this Sep 8, 2022
@pileks pileks marked this pull request as ready for review September 8, 2022 15:56
Niraj-Kamdar
Niraj-Kamdar previously approved these changes Sep 8, 2022
dOrgJelli
dOrgJelli previously approved these changes Sep 15, 2022
@dOrgJelli dOrgJelli merged commit 796cf8a into origin-dev Sep 15, 2022
@dOrgJelli dOrgJelli deleted the pileks-allow-user-to-get-plugin-config-from-client branch April 10, 2023 17:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

allow user to get plugin configs from the client or Web3ApiClientConfig object
6 participants