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

Plugin Extensions: Custom limits for extensions-per-plugin #69430

Merged
merged 5 commits into from
Jun 2, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,11 @@ import { isPluginExtensionComponent, isPluginExtensionLink } from './utils';
export type GetPluginExtensions<T = PluginExtension> = ({
extensionPointId,
context,
limitPerPlugin,
}: {
extensionPointId: string;
context?: object | Record<string | symbol, unknown>;
limitPerPlugin?: number;
}) => {
extensions: T[];
};
Expand Down
1 change: 1 addition & 0 deletions public/app/features/dashboard/utils/getPanelMenu.ts
Original file line number Diff line number Diff line change
Expand Up @@ -277,6 +277,7 @@ export function getPanelMenu(
const { extensions } = getPluginExtensions({
extensionPointId: PluginExtensionPoints.DashboardPanelMenu,
context: createExtensionContext(panel, dashboard),
limitPerPlugin: 2,
});

if (extensions.length > 0 && !panel.isEditing) {
Expand Down
1 change: 0 additions & 1 deletion public/app/features/plugins/extensions/constants.ts

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -63,33 +63,6 @@ describe('createRegistry()', () => {
);
});

it('should register maximum 2 extensions / plugin / placement', () => {
const registry = createPluginExtensionRegistry([{ pluginId, extensionConfigs: [link1, link1, link1] }]);

expect(Object.getOwnPropertyNames(registry)).toEqual([placement1]);

// Placement 1
expect(registry[placement1]).toHaveLength(2);
expect(registry[placement1]).toEqual(
expect.arrayContaining([
expect.objectContaining({
pluginId,
config: {
...link1,
configure: expect.any(Function),
},
}),
expect.objectContaining({
pluginId,
config: {
...link1,
configure: expect.any(Function),
},
}),
])
);
});

it('should not register link extensions with invalid path configured', () => {
const registry = createPluginExtensionRegistry([
{ pluginId, extensionConfigs: [{ ...link1, path: 'invalid-path' }, link2] },
Expand Down
Original file line number Diff line number Diff line change
@@ -1,14 +1,11 @@
import type { PluginPreloadResult } from '../pluginPreloader';

import { MAX_EXTENSIONS_PER_POINT } from './constants';
import { ExtensionsPerPlugin } from './extensionsPerPlugin';
import type { PluginExtensionRegistryItem, PluginExtensionRegistry } from './types';
import { deepFreeze, logWarning } from './utils';
import { isPluginExtensionConfigValid } from './validators';

export function createPluginExtensionRegistry(pluginPreloadResults: PluginPreloadResult[]): PluginExtensionRegistry {
const registry: PluginExtensionRegistry = {};
const extensionsPerPlugin = new ExtensionsPerPlugin();

for (const { pluginId, extensionConfigs, error } of pluginPreloadResults) {
if (error) {
Expand All @@ -19,13 +16,6 @@ export function createPluginExtensionRegistry(pluginPreloadResults: PluginPreloa
for (const extensionConfig of extensionConfigs) {
const { extensionPointId } = extensionConfig;

if (!extensionsPerPlugin.allowedToAdd(extensionConfig)) {
logWarning(
`"${pluginId}" plugin has reached the limit of ${MAX_EXTENSIONS_PER_POINT} for "${extensionPointId}", skip registering extension "${extensionConfig.title}".`
);
continue;
}

if (!extensionConfig || !isPluginExtensionConfigValid(pluginId, extensionConfig)) {
continue;
}
Expand Down
33 changes: 0 additions & 33 deletions public/app/features/plugins/extensions/extensionsPerPlugin.ts

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,52 @@ describe('getPluginExtensions()', () => {
);
});

test('should not limit the number of extensions per plugin by default', () => {
// Registering 3 extensions for the same plugin for the same placement
const registry = createPluginExtensionRegistry([{ pluginId, extensionConfigs: [link1, link1, link1, link2] }]);
const { extensions } = getPluginExtensions({ registry, extensionPointId: extensionPoint1 });

expect(extensions).toHaveLength(3);
expect(extensions[0]).toEqual(
expect.objectContaining({
pluginId,
type: PluginExtensionTypes.link,
title: link1.title,
description: link1.description,
path: link1.path,
})
);
});

test('should be possible to limit the number of extensions per plugin for a given placement', () => {
const registry = createPluginExtensionRegistry([
{ pluginId, extensionConfigs: [link1, link1, link1, link2] },
{
pluginId: 'my-plugin',
extensionConfigs: [
{ ...link1, path: '/a/my-plugin/declare-incident' },
{ ...link1, path: '/a/my-plugin/declare-incident' },
{ ...link1, path: '/a/my-plugin/declare-incident' },
{ ...link2, path: '/a/my-plugin/declare-incident' },
],
},
]);

// Limit to 1 extension per plugin
const { extensions } = getPluginExtensions({ registry, extensionPointId: extensionPoint1, limitPerPlugin: 1 });

expect(extensions).toHaveLength(2);
expect(extensions[0]).toEqual(
expect.objectContaining({
pluginId,
type: PluginExtensionTypes.link,
title: link1.title,
description: link1.description,
path: link1.path,
})
);
});

test('should return with an empty list if there are no extensions registered for a placement yet', () => {
const registry = createPluginExtensionRegistry([{ pluginId, extensionConfigs: [link1, link2] }]);
const { extensions } = getPluginExtensions({ registry, extensionPointId: 'placement-with-no-extensions' });
Expand Down
17 changes: 16 additions & 1 deletion public/app/features/plugins/extensions/getPluginExtensions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,23 +26,36 @@ import {
type GetExtensions = ({
context,
extensionPointId,
limitPerPlugin,
registry,
}: {
context?: object | Record<string | symbol, unknown>;
extensionPointId: string;
limitPerPlugin?: number;
registry: PluginExtensionRegistry;
}) => { extensions: PluginExtension[] };

// Returns with a list of plugin extensions for the given extension point
export const getPluginExtensions: GetExtensions = ({ context, extensionPointId, registry }) => {
export const getPluginExtensions: GetExtensions = ({ context, extensionPointId, limitPerPlugin, registry }) => {
const frozenContext = context ? getReadOnlyProxy(context) : {};
const registryItems = registry[extensionPointId] ?? [];
// We don't return the extensions separated by type, because in that case it would be much harder to define a sort-order for them.
const extensions: PluginExtension[] = [];
const extensionsByPlugin: Record<string, number> = {};

for (const registryItem of registryItems) {
try {
const extensionConfig = registryItem.config;
const { pluginId } = registryItem;

// Only limit if the `limitPerPlugin` is set
if (limitPerPlugin && extensionsByPlugin[pluginId] >= limitPerPlugin) {
continue;
}

if (extensionsByPlugin[pluginId] === undefined) {
extensionsByPlugin[pluginId] = 0;
}

// LINK
if (isPluginExtensionLinkConfig(extensionConfig)) {
Expand All @@ -67,6 +80,7 @@ export const getPluginExtensions: GetExtensions = ({ context, extensionPointId,
};

extensions.push(extension);
extensionsByPlugin[pluginId] += 1;
}

// COMPONENT
Expand All @@ -84,6 +98,7 @@ export const getPluginExtensions: GetExtensions = ({ context, extensionPointId,
};

extensions.push(extension);
extensionsByPlugin[pluginId] += 1;
}
} catch (error) {
if (error instanceof Error) {
Expand Down