fix: If newData is deeply to the latest state, broadcast the latest state

[icyJoseph]

Attempt to fix #1695

[codesandbox-ci]

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 990f8e0:

Sandbox	Source
SWR-Basic	Configuration
SWR-States	Configuration
SWR-Infinite	Configuration
SWR-SSR	Configuration

[shuding]

This is a good fix! And I just took a deeper look and it seems the problem is caused by here:

swr/src/use-swr.ts

Line 372 in 4663154

compare(stateRef.current.data, updatedData)

When they're not equal, we broadcast the new data even if the new data is not defined. So another way to fix it is adding:

isUndefined(updatedData) ||

to that line of code.

[icyJoseph]

Do you have any preference? I guess one if branch vs if-else is more terse?

Also, I commented heavily here, but perhaps I should revert to the original comment?

[icyJoseph]

I may have misunderstood but

if (isUndefined(newData) || !compare(stateRef.current.data, newData)) {
          newState.data = newData
 }

Still requires the else block right (the test still fails otherwise)? We need to bind newState.data to stateRef.current.data.

[shuding]

Oh sorry I meant adding it here:

      setState(
        mergeObjects(
          {
            error: updatedError,
            isValidating: updatedIsValidating
          },
          // Since `setState` only shallowly compares states, we do a deep
          // comparison here.
+         isUndefined(newData) || compare(stateRef.current.data, updatedData)
            ? UNDEFINED
            : {
                data: updatedData
              }
        )
      )

I prefer this for now because it is more universal, so we have a way to call broadCastState() but skip updating the data, if it's undefined.

[icyJoseph]

I tried that too, and it caused a couple other tests to failed, iirc.

Not on my laptop now, but I'll retry first thing in the morning if that's ok.

I also agree that placing the check there seems more correct.

[shuding]

Yeah, if that doesn’t work we can publish your original fix for sure 👍

[icyJoseph]

The test cases that stop working are:

● useSWR - local mutation › should be able to mutate data to undefined
● useSWR - local mutation › should be able to mutate data to undefined asynchronously

The test description kinda makes it clear why, because when mutating to undefined, with the proposed changed we'd be broadcasting the stale state instead.

Since the proposal didn't work out, I also tried:

 isUndefined(updatedData)
            ? { data: UNDEFINED }
            : compare(stateRef.current.data, updatedData)
            ? UNDEFINED
            : {
                data: updatedData
              }

But this breaks two tests 😅 the first one is introduced on this MR, it implicitly reintroduces the initial issue.

● useSWR - loading › should avoid extra rerenders is the data is the `same` 
● useSWR - configs › should read the config fallback from the context

I think it'd be an ok trade-off to patch this now, and investigate further.

[shuding]

Thank you, yeah that makes total sense!