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

Wrong data passed to custom compare function in version 2.0.x #2375

Closed
fscali opened this issue Jan 23, 2023 · 8 comments · Fixed by #2387
Closed

Wrong data passed to custom compare function in version 2.0.x #2375

fscali opened this issue Jan 23, 2023 · 8 comments · Fixed by #2387
Labels
bug Something isn't working good first issue Good for newcomers

Comments

@fscali
Copy link

fscali commented Jan 23, 2023

Bug report

Description / Observed Behavior

When I configure a custom compare function, the parameters that are passed to my function at runtime are booleans instead of the expected custom type

Expected Behavior

The expected behaviour is that my custom compare functions receives the expected type returned by the fetcher.

Repro Steps / Code Example

compare(a, b) {
        if (typeof a === 'undefined' || typeof b === 'undefined') {
          return false;
        } else if (typeof a === 'object' && typeof b === 'object') {
          const mappedA = a.results.map((r) => r.id);
          const mappedB = a.results.map((r) => r.id);
          return isEqual(mappedA, mappedB);
        } else {
          return a == b;
        }
      },

Additional Context

SWR version: 2.0.1
Framework: Next.js 12.3.1

@koba04
Copy link
Collaborator

koba04 commented Jan 24, 2023

@fscali Could you provide a CodeSandbox project to reproduce this?

@koba04 koba04 added bug Something isn't working and removed reproduction needed labels Jan 25, 2023
@koba04
Copy link
Collaborator

koba04 commented Jan 25, 2023

@fscali Thank you!

@promer94 promer94 added the good first issue Good for newcomers label Jan 26, 2023
@promer94
Copy link
Collaborator

Compare fn is now not only used for comparing data but also other state field (error, isLoading, isValidating).

swr/core/use-swr.ts

Lines 110 to 125 in ad7bb15

const isEqual = (prev: State<Data, any>, current: State<Data, any>) => {
let equal = true
for (const _ in stateDependencies) {
const t = _ as keyof StateDependencies
if (!compare(current[t], prev[t])) {
if (t === 'data' && isUndefined(prev[t])) {
if (!compare(current[t], returnedData)) {
equal = false
}
} else {
equal = false
}
}
}
return equal
}

@Retrospection
Copy link
Contributor

I'd like try solve this~

Retrospection pushed a commit to Retrospection/swr that referenced this issue Jan 27, 2023
Retrospection pushed a commit to Retrospection/swr that referenced this issue Jan 27, 2023
Retrospection pushed a commit to Retrospection/swr that referenced this issue Jan 27, 2023
promer94 pushed a commit to Retrospection/swr that referenced this issue Jan 27, 2023
promer94 added a commit to promer94/swr that referenced this issue Jan 27, 2023
huozhi pushed a commit that referenced this issue Jan 27, 2023
* test: add test case for #2375

* chore: clean up
@fscali
Copy link
Author

fscali commented Jan 28, 2023

Hi, If I undersand well this bug should have been fixed by 2.0.3 but I still get the error in the sandbox I provided..

@promer94
Copy link
Collaborator

try {
          return _.isEqual(
            a!.map((val) => val.id),
            b!.map((val) => val.id)
          );
        } catch (_) {
          return false;
        }

Your compare fn is buggy, it does not have the correct behaviour as

undefined,udefined => true 
undefined, Data => false
Data,undefined => false
Data, Data => boolean

@fscali
Copy link
Author

fscali commented Jan 29, 2023

Thank you @promer94 ...I tried but same error (see same codesandbox). In version 1.3 it works..

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants