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.

Sign up for GitHub

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

Jump to bottom

Plugins: Refresh plugin info after installation #75074

Merged
merged 2 commits into from Sep 21, 2023
Merged

Plugins: Refresh plugin info after installation #75074

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b198276
Plugins: Refresh plugin info after installation
oshirohugo Sep 19, 2023
5b2fc65
Fix format
oshirohugo Sep 21, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
12 changes: 11 additions & 1 deletion public/app/features/plugins/admin/components/InstallControls/InstallControlsButton.tsx
View file
Open in desktop
Expand Up @@ -9,7 +9,14 @@ import { useQueryParams } from 'app/core/hooks/useQueryParams';
import { removePluginFromNavTree } from 'app/core/reducers/navBarTree';
import { useDispatch } from 'app/types';

import { useInstallStatus, useUninstallStatus, useInstall, useUninstall, useUnsetInstall } from '../../state/hooks';
import {
useInstallStatus,
useUninstallStatus,
useInstall,
useUninstall,
useUnsetInstall,
useFetchDetailsLazy,
} from '../../state/hooks';
import { trackPluginInstalled, trackPluginUninstalled } from '../../tracking';
import { CatalogPlugin, PluginStatus, PluginTabIds, Version } from '../../types';

Expand All @@ -36,6 +43,7 @@ export function InstallControlsButton({
const install = useInstall();
const uninstall = useUninstall();
const unsetInstall = useUnsetInstall();
const fetchDetails = useFetchDetailsLazy();
const [isConfirmModalVisible, setIsConfirmModalVisible] = useState(false);
const showConfirmModal = () => setIsConfirmModalVisible(true);
const hideConfirmModal = () => setIsConfirmModalVisible(false);
Expand All @@ -57,6 +65,8 @@ export function InstallControlsButton({
const onInstall = async () => {
trackPluginInstalled(trackingProps);
const result = await install(plugin.id, latestCompatibleVersion?.version);
// refresh the store to have the new installed plugin
await fetchDetails(plugin.id);
if (!errorInstalling && !('error' in result)) {
appEvents.emit(AppEvents.alertSuccess, [`Installed ${plugin.name}`]);
if (plugin.type === 'app') {
Expand Down
6 changes: 6 additions & 0 deletions public/app/features/plugins/admin/state/hooks.ts
View file
Open in desktop
Expand Up @@ -133,6 +133,12 @@ export const useFetchDetails = (id: string) => {
}, [plugin]); // eslint-disable-line
};

export const useFetchDetailsLazy = () => {
const dispatch = useDispatch();

return (id: string) => dispatch(fetchDetails(id));
};

Copy link
Contributor

Choose a reason for hiding this comment

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

Although this should work, maybe what we could do instead is to add a way to force-fetch with the existing useFetchDetails() hook - in that case we would only have a single way for fetching plugin details using hooks.

E.g.:

export const useFetchDetails = (id: string, { force = false } = {}) => {
  const dispatch = useDispatch();
  const plugin = useSelector((state) => selectById(state, id));
  const isNotFetching = !useSelector(selectIsRequestPending(fetchDetails.typePrefix));
  const shouldFetch = force || (isNotFetching && plugin && !plugin.details);

  useEffect(() => {
    shouldFetch && dispatch(fetchDetails(id));
  }, [plugin]); // eslint-disable-line
};

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@leventebalogh tks for your review.

I tried a similar approach before, removing the !plugin.details from shouldFetch caused a infinite loop. The main problem is that fetchDetails is inside useEffect in this existing hook. That's why I went for the lazy fetch.

The suggested approach will cause the same.
Just to check if I'm not doing anything wrong: I changed the useFetchDetails, as you suggested, and then added the hook in InstallControlsButton component right before the first useEffect that is already there.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, you are actually right - I missed that useFetchDetails() is not returning with a function.
Let's leave it as it is 👍

export const useDisplayMode = () => {
const dispatch = useDispatch();
const displayMode = useSelector(selectDisplayMode);
Expand Down