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

fix refreshInstance type and remove return #8478

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

fix refreshInstance type and remove return #8478

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
08ee2df
fix refreshInstance type and remove return
matmair Nov 12, 2024
528f24f
Merge branch 'master' into fix-promise
SchrodingersGat Nov 14, 2024
2f1b8d2
Add seperate refresh function with promise as return
matmair Nov 14, 2024
aac8729
Merge branch 'fix-promise' of https://github.com/matmair/InvenTree in…
matmair Nov 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 11 additions & 2 deletions src/frontend/src/hooks/UseInstance.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,8 @@ import { type PathParams, apiUrl } from '../states/ApiState';
export interface UseInstanceResult {
instance: any;
setInstance: (instance: any) => void;
refreshInstance: () => Promise<QueryObserverResult<any, any>>;
refreshInstance: () => void;
refreshInstancePromise: () => Promise<QueryObserverResult<any, any>>;
Comment on lines -11 to +12
Copy link
Contributor

@wolflu05 wolflu05 Nov 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

What is the reason for this change? You don't need to await the Promise (use its return value) if you don't want to where you use refreshInstance?

Copy link
Contributor

@wolflu05 wolflu05 Nov 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Dosen something like:

 return {
    sessionData,
    setSessionData,
    sessionId,
-    refreshSession
+    refreshSession: () => { refreshSession() },
    sessionQuery,
    status,

would fix the error to, but without cluttering the useInstance hook with unnecessary functions. (Maybe the function needs to be wrapped in a useCallback too.)

Copy link
Member Author

@matmair matmair Nov 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I do not see how this would fix the issue - please look at the errors linked in the description

Copy link
Member

Choose a reason for hiding this comment

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

The additional wrapper promise function does seem clunky.. I'm sure there should be a better way to suppress the warning

Copy link
Member Author

Choose a reason for hiding this comment

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

I do note really care how this is solved but there are 28 issues with severity middle caused by the underlying change - those should be addresses https://sonarcloud.io/project/issues?rules=typescript%3AS6544&issueStatuses=OPEN%2CCONFIRMED&id=inventree_InvenTree

instanceQuery: any;
requestStatus: number;
isLoaded: boolean;
Expand All @@ -23,6 +24,7 @@ export interface UseInstanceResult {
* To use this hook:
* const { instance, refreshInstance } = useInstance(url: string, pk: number)
*/

export function useInstance<T = any>({
endpoint,
pk,
Expand Down Expand Up @@ -112,12 +114,19 @@ export function useInstance<T = any>({
);
}, [instanceQuery]);

const refreshInstance = useCallback(() => instanceQuery.refetch(), []);
const refreshInstance = useCallback(() => {
instanceQuery.refetch();
}, []);

const refreshInstancePromise = useCallback(() => {
return instanceQuery.refetch();
}, []);

return {
instance,
setInstance,
refreshInstance,
refreshInstancePromise,
instanceQuery,
requestStatus,
isLoaded
Expand Down
3 changes: 2 additions & 1 deletion src/frontend/src/pages/stock/StockDetail.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,7 @@ export default function StockDetail() {
const {
instance: stockitem,
refreshInstance,
refreshInstancePromise,
instanceQuery,
requestStatus
} = useInstance({
Expand Down Expand Up @@ -614,7 +615,7 @@ export default function StockDetail() {
},
onFormSuccess: () => {
const partId = stockitem.part;
refreshInstance().catch(() => {
refreshInstancePromise().catch(() => {
// Part may have been deleted - redirect to the part detail page
navigate(getDetailUrl(ModelType.part, partId));
});
Expand Down
Loading