frontend: Introduce react-query and v2 API

[sniok]

fixes #1700

This PR introduces react-query as the library to perform and coordinate requests to the backend.
It brings nice quality of life improvements like caching, deduplicating requests, error handling, convenient react hooks.

For KubeObject classes, new methods were added: useQuery (alternative to useGet), useListQuery (alternative to useList). The old methods are left as-is (marked as deprecated) for compatibility with plugins.

Some requests were not converted to limit the scope of this PR. Things like health and config requests for example, where caching might introduce problems, are left to do in next PRs.

Testing done

• Manually checked the application
• Ensured that the apiProxy exports stay the same after refactor (all the exported functions/types are present and unchanged)
• Manually checked plugins/examples
• Manually checked plugins repo plugins
• Manually checked app compiled with make app-linux
• Checked headlamp-plugin page reloaded when plugin changed.
• Load tested with load-test scripts in Edge browser

[illume]

Very nice work. I'm looking forward to these improvements.

I don't have time for an in depth review right now... But my general questions of all PRs: are there docs, tests for new code, atomic commits? Can anything be broken out into separate PRs? How does it affect plugins? Adopting a new technology... were alternatives evaluated and is the decision reversable/or leaking to users? ( I guess @reduxjs/toolkit/query is the alternative here ). Is there something broken/deprecated and how is that going to be communicated?

[skoeva]

Test coverage looks good, docs are concise and readable, and big thanks for splitting the commits across the different PRs. Monumental task, great work!

[illume]

🎉 thanks

(please fix those issues @skoeva mentioned, then we can merge)

[illume]

I went through and checked the remaining open convos were addressed and resolved them. There's only the two notes left.

[illume]

marked as deprecated

What is the plan for deprecation and migration? How long are we keeping these around?

I think users should have a "@see newfunction and an example of how to change to the new function. So when they hover over their striked out deprecated function they can see how to change it very easily.

It would make sense to update the examples/ plugins in this PR so we can more easily test them.

---

Looking at the uses of useListQuery... why does the function signature have to be different?

Old use of useList:

  const [pods, podsFetchError] = Pod.useList({
    namespace,
    labelSelector: getPodsSelectorFilter(service),
  });

New use of useListQuery.

  const { items: pods, error: podsFetchError } = Pod.useListQuery({
    namespace,
    queryParams: {
      labelSelector: getPodsSelectorFilter(service),
    },
  });

• I don't see benefit for this function signature change as it's used. Theoretically if we want to extend the returned values it's a bit nicer. But in terms of how it's used... it doesn't seem to be a gain. So can you please explain that?
• Can useList be implemented in terms of useListQuery? Allowing us to drop that old code?

[sniok]

What is the plan for deprecation and migration? How long are we keeping these around?

The old functions are probably used a lot, first we can mark them deprecated in the comment, then probably console warning, then removal. But I think we'll keep them for quite a while

Looking at the uses of useListQuery... why does the function signature have to be different?

Old use of useList:

  const [pods, podsFetchError] = Pod.useList({
    namespace,
    labelSelector: getPodsSelectorFilter(service),
  });

New use of useListQuery.

  const { items: pods, error: podsFetchError } = Pod.useListQuery({
    namespace,
    queryParams: {
      labelSelector: getPodsSelectorFilter(service),
    },
  });

• I don't see benefit for this function signature change as it's used. Theoretically if we want to extend the returned values it's a bit nicer. But in terms of how it's used... it doesn't seem to be a gain. So can you please explain that?

There are already more returned values than items and error. Things like status or refetch or data that contains KubeList (if you want to keep track of resourceVersion of the list). Returning tuple makes sense if we're sure that nothing else will be added but it's not the case here.
The queryParams parameter is more explicit now, which I think is easier to understand

• Can useList be implemented in terms of useListQuery? Allowing us to drop that old code?

I considered it, but the behavior of the functions is slightly different, useList will always send a request while useListQuery will sometimes use cached data. Together with the function signature change I think introducing a new function is justified.

[sniok]

Pushed the suggested changes (marked them as resolved as they're straightforward) and created an issue for updating the plugin examples #2322

[illume]

I don't see benefit for this function signature change as it's used. Theoretically if we want to extend the returned values it's a bit nicer. But in terms of how it's used... it doesn't seem to be a gain. So can you please explain that?

There are already more returned values than items and error. Things like status or refetch or data that contains KubeList (if you want to keep track of resourceVersion of the list). Returning tuple makes sense if we're sure that nothing else will be added but it's not the case here.

At the moment I can't see any of the extra pieces of data used. The most common usage is like this:

  const [pods, podsFetchError] = Pod.useList({
    namespace,
    labelSelector: getPodsSelectorFilter(service),
  });

Which is simpler to use than this:

  const { items: pods, error: podsFetchError } = Pod.useListQuery({
    namespace,
    queryParams: {
      labelSelector: getPodsSelectorFilter(service),
    },
  });

The queryParams parameter is more explicit now, which I think is easier to understand

The queryParams is redundant here, because "Selector" is already signifying that it's a selector.

In the future if users of these functions do use the extra information returned... at that point a different input could allow returning that extra info in an object. Which would keep compatibility with existing call sites.

Can useList be implemented in terms of useListQuery? Allowing us to drop that old code?

I considered it, but the behavior of the functions is slightly different, useList will always send a request while useListQuery will sometimes use cached data. Together with the function signature change I think introducing a new function is justified.

The major issue with this is that we have to keep maintaining two versions of code. For maintenance it would be better to drop that old code now.

The problem with having a cached and non-cached view of the data is that it's inconsistent. This is why I think it's better to have them both use the cached version. I hope that the watches update that cached data more quickly than a new request would make (or at least just as quickly).

[illume]

What is the idea with adding /api/ inside lib/k8s?

frontend/src/lib/k8s/apiProxy/portForward.ts → frontend/src/lib/k8s/api/v1/portForward.ts

Could you also please explain the idea and the plan with having a v1 and v2 folder?

[sniok]

At the moment I can't see any of the extra pieces of data used. The most common usage is like this:

  const [pods, podsFetchError] = Pod.useList({
    namespace,
    labelSelector: getPodsSelectorFilter(service),
  });

Which is simpler to use

Yes, right now in most places we're just using result and error.
But now we use react-query and there's more information that we can (and should) use.

For example, now that we have caching, a common feature that improves UX is some kind of loading indicator that appears when data is refetched (while cached data is displayed).

There's no way to represent this with just [data,error] tuple. Currently we use null to represent that data is loading but that is not a good practice (while appearing simple, sometimes null might be a valid response from backend). A much better solution is having a status variable.

If you look at the current type definition of useList you'll see

[any[], ApiError | null, (items: any[]) => void, (err: ApiError | null) => void];

which is not always obvious, for example guessing from just the type what the last item does is not clear.
Having a well documented object with clearly named properties is much easier to understand and to work with.

The queryParams is redundant here, because "Selector" is already signifying that it's a selector.

Query parameters is a set of properties that is separate from the other parameters. Just like it is separated in Kubernetes documentation, PodList example. People that are already familiar with Kubernetes API would probably find it easier to work with.

Also current useGet has queryParams separate https://github.com/headlamp-k8s/headlamp/blob/main/frontend/src/lib/k8s/cluster.ts#L662

In the future if users of these functions do use the extra information returned... at that point a different input could allow returning that extra info in an object. Which would keep compatibility with existing call sites.

Functions which type definition changes based on parameters passed are usually hard to work with, I'd prefer avoiding doing that.

The major issue with this is that we have to keep maintaining two versions of code. For maintenance it would be better to drop that old code now.

The problem with having a cached and non-cached view of the data is that it's inconsistent. This is why I think it's better to have them both use the cached version. I hope that the watches update that cached data more quickly than a new request would make (or at least just as quickly).

I could replace old functions completely, preserving function definitions. That would require some additional effort

[sniok]

What is the idea with adding /api/ inside lib/k8s?

that's where apiProxy was, then it was refactored apiProxy -> api/{factories,requests,etc..}/

frontend/src/lib/k8s/apiProxy/portForward.ts → frontend/src/lib/k8s/api/v1/portForward.ts

Could you also please explain the idea and the plan with having a v1 and v2 folder?

v1 is what we currently have, v2 is new version of doing requests to the backend and react-query stuff
after a while I'd expect to move all functionality from v1 to v2 and then remove v1

[sniok]

Pushed a big change, instead of adding new functions, they're now replacing old useGet and useList. But with some iterator trickery it is now possible to do both const [items,error] = useList and const { data, items, status, ..etc } = useList at the same time

[illume]

hi.

I’ll leave this to @joaquimrocha to give his sign off and merge, because he was reviewing it before.

Looks good to me. The only slight concerns I have left are about the “api” name, and introducing v1/v2, but will leave that up to you both to consider/decide.

thanks!!

[joaquimrocha]

Left a comment. I think this PR would be easier to review if we had the move into v1 as a previous, separate commit.
About the versioned folders. I am totally in favor of having them so we can eventually move to a new version that has little to do with the previous one. And I am also happy to drop the previous version of apiProxy that was doing things like sequentially trying different k8s APIs without remembering any. However, in this case, unless I misunderstood it, it looks like we keep using most of the v1 (apply function, scale API, etc.) but we have the v2 being used for replacing the internals of the useGet/useList (and others), but it reads more like we are extending the previous API rather than deprecating it.

So I want to understand better the justification for having the 2 versions. Is v1 going to be deprecated? When do we get a replacement API for things like scale, apply, ...?

[sniok]

it looks like we keep using most of the v1 (apply function, scale API, etc.) but we have the v2 being used for replacing the internals of the useGet/useList (and others), but it reads more like we are extending the previous API rather than deprecating it.

Here's the state of v1 and v2. The plan is to have everything in v2 eventually. Getting/watching list/objects is the most impactful for UX and it is now using react-query.

	v1	v2
Requests to backend/cluster	✅	✅
Get object/list	✅[0]	✅[1]
Watch object/list	✅[0]	✅[2]
Apply	✅	❌
Drain node	✅	❌
Metrics	✅	❌
Plugin	✅	❌
Port Forward	✅	❌
Scale	✅	❌
Token	✅	❌

[0] - This is still present in the code and available via apiProxy/index.ts file for compatibility, but it's not used for requests in our codebase anymore
[1] - These requests were greatly simplified by refactoring away apiFactories and using named arguments (objects) instead of positional arguments
[1][2] - react-query stuff

Now those API that are not yet in v2 are quite simple and can be included in v2 with minimal changes.

[sniok]

Pushed an update that moves v1 into a new commit, updates websocket logic

[joaquimrocha]

@sniok Okay. I still think it's a bit weird we are branching the version when we are not marking anything as deprecated and we don't have 100% coverage for what we had in v1, but I can see a way where we add those along the way, so I have merged the changes.