-
Notifications
You must be signed in to change notification settings - Fork 5
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
Component table improvements #88
Conversation
I added the `fetchPages` function in utils to make this easy to do just about anywhere. The implementation in the view was broken due to JS variable scoping *and* the fact that `httpGet` is sync but async :/ The `fetchPages` function is built on top of `httpGet` so it has the same weirdness wrto. sync/async. It is best to think of it as a background continuation since it uses `setTimeout` to queue up the next page fetch. It invokes a callback when processing API responses and includes a "done" flag so you can keep track of whether it is processing or not at the component level. See the `ComponentList` for the expected usage pattern.
This is a single column sort which is a lot easier to implement and understand (from the user perspective) than the multicolumn sort implemented in a few other places. I also included the option to specify the default sort as a property so that the UI can be sorted and make sense on the initial render.
None of these are paginated ... yet. I expect that the components report will end up paginated in the near future.
Noticed this when I was editing components.
const nextLink = Object.hasOwn(links, 'next') ? links.next[0] : null | ||
onDataReceived(data, nextLink === null) | ||
if (nextLink !== null) { | ||
setTimeout(localFetch, 25, nextLink) |
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.
Why after a timeout? Since this is onSuccess, the previous request has already completed. Is there a reason to wait a little longer on top of that?
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.
I think that I did this to avoid tight looping (eg, let the browser and the server breath a little). Not completely sure if it is necessary or not. I can't recall if there is a stack depth issue or not since localFetch
results in onSuccess
being called. I think that there is some async buried in there somewhere that prevents that though 😕
This should also sever the closure chain that would otherwise result... I think
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.
I don't think there is a stack depth issue, setTimeout
queues up the callback out of the main thread, so the enclosing onSuccess
function finishes, and the localFetch
callback gets executed outside of it.
Co-authored-by: Alexander Campbell <[email protected]>
Once I added some components to a project, I noticed that the multi-page fetch in
ComponentList
was broken due to my misunderstanding of how that code worked. I fixed it by adding a clean way to retrieve a paginated response from the API.The new
fetchPages
function in utils retrieves a page, invokes a callback to inject the data, and retrieves the next page if anext
link header is in the response. The callback is also passed a "finished" flag so that the component can update its "fetching" state. There is a little more to it, but that is the gist of the function. See theComponentList
changes in 743750f for the implementation. I switched theReport
component over to usingfetchPages
so that we can paginate them in the future.I also added sorting to the
NavigableTable
so that the user can sort the Components table and adjusted some column widths.