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

feat(develop): add refetch interval to useiceberg hook #13488

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

pauldkn
Copy link
Contributor

@pauldkn pauldkn commented Oct 8, 2024

desc: allow refetchInterval as prop to the useIceberg hook
Issue 13513

Question Answer
Branch? develop
Bug fix? no
New feature? yes
Breaking change? no
Tickets Issue #13513
License BSD 3-Clause
  • Try to keep pull requests small so they can be easily reviewed.
  • Commits are signed-off
  • Only FR translations have been updated
  • Branch is up-to-date with target branch
  • Lint has passed locally
  • Standalone app was ran and tested locally
  • Ticket reference is mentioned in linked commits (internal only)
  • Breaking change is mentioned in relevant commits

Description

Related

@pauldkn pauldkn requested review from a team as code owners October 8, 2024 07:52
@github-actions github-actions bot added the feature New feature label Oct 8, 2024
  desc: allow refetchInterval as prop

Signed-off-by: Paul Dickerson <[email protected]>
@pauldkn pauldkn force-pushed the feat/update-iceberg-hook-params branch from ff5d3c4 to f3342eb Compare October 8, 2024 09:55
Copy link

sonarcloud bot commented Oct 8, 2024

@pauldkn pauldkn linked an issue Oct 9, 2024 that may be closed by this pull request
1 task
Comment on lines 10 to +16
interface IcebergV2Hook {
queryKey: string[];
defaultSorting?: ColumnSort;
refetchInterval?:
| number
| false
| ((query: Query) => number | false | undefined);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
interface IcebergV2Hook {
queryKey: string[];
defaultSorting?: ColumnSort;
refetchInterval?:
| number
| false
| ((query: Query) => number | false | undefined);
interface IcebergV2Hook {
defaultSorting?: ColumnSort;
} extends UndefinedInitialDataInfiniteOptions<IcebergFetchResultV2<T>

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's good If we can add all query options on params and decompose is on useInfiniteQuery

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see what you'd like to do, but it goes beyond this issue, doesn't it?
It may have been intentional not to expose every query option on this hook, which is why I'm only requesting the refetchInterval option, the minimum needed here.
However, an issue could certainly be opened to expose every option on icebergV2 or V6 if necessary 👍

Copy link
Contributor

@tristanwagner tristanwagner Oct 10, 2024

Choose a reason for hiding this comment

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

just for your information: in my experience using refreshinterval with useInfiniteQuery without selecting only the actual payload (in data.pages) with select and omitting the page params caused rerenders even if the data didn't changed after refresh (because the cursor changes even tho data is the same) so be careful about that for your use case you might need to use select aswell. And it's even more the case with this hook since there is some state update on every data change which should be conpletely removed

});

useEffect(() => {
const flatten = data?.pages.map((page) => page.data).flat();
const flatten = data?.pages
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not use flatMap() directly ?

@github-actions github-actions bot added the has conflicts Has conflicts to resolve before merging label Oct 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature has conflicts Has conflicts to resolve before merging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[manager-components]: Add refetchInterval on useIcebergV2
5 participants