fix: clear refresh timer on effects change and check it on starting new timer

[huozhi]

Fix #852

Description

swr/src/use-swr.ts

Lines 687 to 697 in 00c5847

	// Read the latest refreshInterval
	if (configRef.current.refreshInterval) {
	timer = setTimeout(tick, configRef.current.refreshInterval)
	}
	}
	if (configRef.current.refreshInterval) {
	timer = setTimeout(tick, configRef.current.refreshInterval)
	}
	return () => {
	if (timer) clearTimeout(timer)
	}

when timer is been cancelled, but to the timer procedure, it cannot know if the last timer is cancelled. so it started a new timer again.

steps:
start timer > revalidate > timer is cancelled due to effects > but still start a new timer (with legacy revalidate).

Changes

mark timer as null when cancel it and the cancel state is kept as condition. then swr can avoid to start a new timer again.

[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 a8d3549:

Sandbox	Source
SWR-Basic	Configuration
SWR-States	Configuration
SWR-Infinite	Configuration
youthful-elgamal-g1n4u	Issue #852

[shuding]

This is a good catch! Indeed there's an async process inside tick so it's not safe to guarantee that cancel is safe. But I don't understand why you need an object to bind the latest timer value to the closure, I suppose just timer would work too (by checking if it's null on line 688)?

[huozhi]

@shuding yeah re-test it, it works well! thanks

[shuding]

Thank you so much 😊

[promer94]

It seems i am a little late 😂. I just find a solution

    // Read the latest refreshInterval and make sure the key is not changed
      if (configRef.current.refreshInterval && key === keyRef.current) {
        timer = setTimeout(tick, configRef.current.refreshInterval)
      }

[huozhi]

@promer94 that's a very good point! but the effects clean up can due to several change, we cannot guarantee the key will be the only factor in the long time.

[promer94]

Another problem is that the actual refreshInterval is the response time + refreshInterval option

We should mention this in the docs

[shuding]

Seems some interval tests are failing