-
Notifications
You must be signed in to change notification settings - Fork 15
[BREAKINGCHANGE] fetch plugin based on version #128
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
base: main
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -23,7 +23,9 @@ import { | |||||
| } from '../../model'; | ||||||
| import { PluginRegistryContext } from '../../runtime'; | ||||||
| import { useEvent } from '../../utils'; | ||||||
| import { usePluginIndexes, getTypeAndKindKey } from './plugin-indexes'; | ||||||
| import { usePluginIndexes, PluginCompoundKey } from './plugin-indexes'; | ||||||
|
|
||||||
| export type PluginPreference = { kind: PluginType; name: string; version: string; registry: string }; | ||||||
|
|
||||||
| export interface PluginRegistryProps { | ||||||
| pluginLoader: PluginLoader; | ||||||
|
|
@@ -62,28 +64,79 @@ export function PluginRegistry(props: PluginRegistryProps): ReactElement { | |||||
| }); | ||||||
|
|
||||||
| const getPlugin = useCallback( | ||||||
| async <T extends PluginType>(kind: T, name: string): Promise<PluginImplementation<T>> => { | ||||||
| // Get the indexes of the installed plugins | ||||||
| async <T extends PluginType>(compoundKeyObj: PluginCompoundKey<T>): Promise<PluginImplementation<T>> => { | ||||||
| const pluginIndexes = await getPluginIndexes(); | ||||||
| let resource: PluginModuleResource | undefined = undefined; | ||||||
| let pluginModule: Record<string, Plugin<UnknownSpec>> | undefined; | ||||||
| const { kind, name, version, registry } = compoundKeyObj; | ||||||
|
|
||||||
| /** | ||||||
| * The logic has been developed based on the following discussion: | ||||||
| * https://github.com/perses/shared/pull/128#discussion_r3200721179 | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. maybe is better to add the rationale rather than a link to a discussion. |
||||||
| */ | ||||||
|
|
||||||
| // Figure out what module the plugin is in by looking in the index | ||||||
| const typeAndKindKey = getTypeAndKindKey(kind, name); | ||||||
| const resource = pluginIndexes.pluginResourcesByNameAndKind.get(typeAndKindKey); | ||||||
| if (resource === undefined) { | ||||||
| if (registry || version) { | ||||||
| let compoundKey = ''; | ||||||
| /** | ||||||
| * likely keys are | ||||||
| * 1- kind:name:registry:version | ||||||
| * 2- kind:name::version | ||||||
| * 3- kind:name:registry: | ||||||
| * The last one should be looked up with startsWith | ||||||
| * Because there might be 2 different versions from the same registry | ||||||
| */ | ||||||
| compoundKey = `${kind}:${name}:${registry}:${version}`; | ||||||
| resource = | ||||||
| pluginIndexes.pluginResourcesByNameAndKind.get(compoundKey) || | ||||||
| pluginIndexes.pluginResourcesByNameAndKind.get( | ||||||
| Array.from(pluginIndexes.pluginResourcesByNameAndKind.keys()).find((key) => | ||||||
| key.startsWith(compoundKey, 0) | ||||||
| ) ?? '' | ||||||
| ); | ||||||
|
|
||||||
| if (resource) { | ||||||
| pluginModule = (await loadPluginModule(resource)) as Record<string, Plugin<UnknownSpec>>; | ||||||
| const plugin = pluginModule?.[`${name}:${registry ?? ''}:${version ?? ''}`]; | ||||||
| if (!plugin) { | ||||||
| throw new Error( | ||||||
| `The ${name} plugin for kind '${kind}' is missing from the ${resource!.metadata.name} plugin module` | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
| ); | ||||||
| } | ||||||
| return plugin as PluginImplementation<T>; | ||||||
| } | ||||||
| } | ||||||
| /** | ||||||
| * Here we need to CRAFT a resource from an EXISTING ONE with empty version and registry | ||||||
| * This way we can rely on the backend to get the latest version | ||||||
| * Remember, an EXISTING ONE is coming from getInstalledPlugins, so it includes the spec.plugins (which is used in subsequent steps) | ||||||
| * To craft a resource: | ||||||
| * 1- we search through the pluginIndexes (which is provided from installedPlugins) to find a resource which starts with kind:name | ||||||
| * 2- in metadata we set the version to empty_string and registry to undefined | ||||||
| * 3- rest of the structure should remain intact | ||||||
| * 4- we pass this crafted resource to loadPluginModule | ||||||
| * 5- since the version and registry are empty the url will be baseUrl/moduleName///mf-manifest.json | ||||||
| * 6- the backend handle the empty version and registry and returns the latest | ||||||
| */ | ||||||
| const resourceKey = Array.from(pluginIndexes.pluginResourcesByNameAndKind.keys()).find((k) => | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this search is non deterministic, it might find an older version rather than the latest which should be the default
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It doesn't really matter if it finds an old one or new one. Why do we need it anyway? Because of the list of plugins it provides. There is a step which loops through the list of plugins to register them individually per module const resourceKey = Array.from(pluginIndexes.pluginResourcesByNameAndKind.keys()).find((k) =>
k.startsWith(`${kind}:${name}:`, 0)
);
resource = pluginIndexes.pluginResourcesByNameAndKind.get(resourceKey || '');
if (!resource) {
throw new Error(`A ${name} plugin for kind '${kind}' is not installed`);
}
const unversionedResource: PluginModuleResource = {
...resource,
metadata: { ...resource.metadata, version: '', registry: undefined },
};Again, I would like to draw your attention to how the legacy code gets the default plugin for the very first time |
||||||
| k.startsWith(`${kind}:${name}:`, 0) | ||||||
| ); | ||||||
| resource = pluginIndexes.pluginResourcesByNameAndKind.get(resourceKey || ''); | ||||||
|
|
||||||
| if (!resource) { | ||||||
| throw new Error(`A ${name} plugin for kind '${kind}' is not installed`); | ||||||
| } | ||||||
|
|
||||||
| // Treat the plugin module as a bunch of named exports that have plugins | ||||||
| const pluginModule = (await loadPluginModule(resource)) as Record<string, Plugin<UnknownSpec>>; | ||||||
| resource.metadata.registry = undefined; | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. instead of mutating directly we should probably load from a copy. |
||||||
| resource.metadata.version = ''; | ||||||
|
|
||||||
| pluginModule = (await loadPluginModule(resource)) as Record<string, Plugin<UnknownSpec>>; | ||||||
|
|
||||||
| // We currently assume that plugin modules will have named exports that match the kinds they handle | ||||||
| const plugin = pluginModule[name]; | ||||||
| if (plugin === undefined) { | ||||||
| const plugin = pluginModule?.[`${name}:${registry ?? ''}:${version ?? ''}`]; | ||||||
| if (!plugin) { | ||||||
| throw new Error( | ||||||
| `The ${name} plugin for kind '${kind}' is missing from the ${resource.metadata.name} plugin module` | ||||||
| `The ${name} plugin for kind '${kind}' is missing from the ${resource!.metadata.name} plugin module` | ||||||
| ); | ||||||
| } | ||||||
|
|
||||||
| return plugin as PluginImplementation<T>; | ||||||
| }, | ||||||
| [getPluginIndexes, loadPluginModule] | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -214,31 +214,40 @@ const getPluginRuntime = (): ModuleFederation => { | |
| return instance; | ||
| }; | ||
|
|
||
| const registerRemote = (name: string, baseURL?: string): void => { | ||
| const registerRemote = (name: string, registry?: string, version?: string, baseURL?: string): void => { | ||
| const pluginRuntime = getPluginRuntime(); | ||
|
|
||
| const existingRemote = pluginRuntime.options.remotes.find((remote) => remote.name === name); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We are breaking the cache as the remotes is now tracked by registryName |
||
| if (!existingRemote) { | ||
| const remoteEntryURL = baseURL ? `${baseURL}/${name}/mf-manifest.json` : `/plugins/${name}/mf-manifest.json`; | ||
| const segments = [registry, version].filter(Boolean); | ||
| const suffix = segments.length ? `/${segments.join('/')}` : ''; | ||
| const prefix = baseURL || '/plugins'; | ||
| const remoteEntryURL = `${prefix}/${name}${suffix}/mf-manifest.json`.replace(/\/+/g, '/'); | ||
|
|
||
| const registryName = `${name}:${registry ?? ''}:${version ?? ''}`; | ||
| pluginRuntime.registerRemotes([ | ||
| { | ||
| name, | ||
| name: registryName, | ||
| entry: remoteEntryURL, | ||
| alias: name, | ||
| alias: registryName, | ||
| }, | ||
| ]); | ||
| } | ||
| }; | ||
|
|
||
| export const loadPlugin = async ( | ||
| moduleName: string, | ||
| pluginName: string, | ||
| baseURL?: string | ||
| ): Promise<RemotePluginModule | null> => { | ||
| registerRemote(moduleName, baseURL); | ||
| export const loadPlugin = async (target: { | ||
| moduleName: string; | ||
| pluginName: string; | ||
| registry?: string; | ||
| version?: string; | ||
| baseURL?: string; | ||
| }): Promise<RemotePluginModule | null> => { | ||
| const { moduleName, pluginName, registry, version, baseURL } = target; | ||
| registerRemote(moduleName, registry, version, baseURL); | ||
|
|
||
| const pluginRuntime = getPluginRuntime(); | ||
|
|
||
| return pluginRuntime.loadRemote<RemotePluginModule>(`${moduleName}/${pluginName}`); | ||
| const registryName = `${moduleName}:${registry ?? ''}:${version ?? ''}`; | ||
| return pluginRuntime.loadRemote<RemotePluginModule>(`${registryName}/${pluginName}`); | ||
| }; | ||
|
|
||
| export function usePluginRuntime({ plugin }: { plugin: PersesPlugin }): { | ||
|
|
@@ -247,6 +256,9 @@ export function usePluginRuntime({ plugin }: { plugin: PersesPlugin }): { | |
| } { | ||
| return { | ||
| pluginRuntime: getPluginRuntime(), | ||
| loadPlugin: () => loadPlugin(plugin.moduleName, plugin.name, plugin.baseURL), | ||
| loadPlugin: (): Promise<RemotePluginModule | null> => { | ||
| const { moduleName, name: pluginName, registry, version, baseURL } = plugin; | ||
| return loadPlugin({ moduleName, pluginName, registry, version, baseURL }); | ||
| }, | ||
| }; | ||
| } | ||
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.
this type is internal only from what I can see.