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

Plugins: Refresh plugin info after installation #75074

Merged
merged 2 commits into from Sep 21, 2023

Conversation

oshirohugo
Copy link
Contributor

What is this feature?

After a plugin installation the redux store was still holding old data, we need to refresh it after installation

Why do we need this feature?

This is necessary to have the right info in shown in plugin catalog right after installation

Who is this feature for?

Plugins users

Which issue(s) does this PR fix?:

Fixes part of #68395

Special notes for your reviewer:

Please check that:

  • It works as expected from a user's perspective.
  • If this is a pre-GA feature, it is behind a feature toggle.
  • The docs are updated, and if this is a notable improvement, it's added to our What's New doc.


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 👍


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.

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

@oshirohugo oshirohugo merged commit 6600dd2 into main Sep 21, 2023
15 checks passed
@oshirohugo oshirohugo deleted the refresh-plugin-info-after-install branch September 21, 2023 11:33
grafana-delivery-bot bot pushed a commit that referenced this pull request Sep 21, 2023
oshirohugo added a commit that referenced this pull request Sep 22, 2023
Plugins: Refresh plugin info after installation (#75074)

(cherry picked from commit 6600dd2)

Co-authored-by: Hugo Kiyodi Oshiro <hugo.oshiro@grafana.com>
@zerok zerok modified the milestones: 10.2.x, 10.2.0 Oct 23, 2023
@oshirohugo oshirohugo self-assigned this Nov 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

3 participants