Skip to content

Don't setTimeout if component is unmounted. #169

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

Merged
ryanbrainard merged 1 commit into from
Apr 20, 2017
Merged

Don't setTimeout if component is unmounted. #169

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion src/components/connect.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -314,7 +314,7 @@ function connect(mapPropsToRequestsToProps, defaults, options) {
return (meta) => {
return (value) => {
let refreshTimeout = null
if (mapping.refreshInterval > 0) {
if (mapping.refreshInterval > 0 && !this._unmounted) {
refreshTimeout = window.setTimeout(() => {
this.refetchDatum(prop, Object.assign({}, mapping, { refreshing: true, force: true }))
}, mapping.refreshInterval)
Expand Down
75 changes: 73 additions & 2 deletions test/components/connect.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -203,7 +203,7 @@ describe('React', () => {
const pending = TestUtils.findRenderedComponentWithType(container, Container)
const startedAt = pending.state.startedAts.testFetch
setImmediate(() => {
expect(startedAt.getTime()).toBeLessThan(new Date().getTime())
expect(startedAt.getTime()).toBeLessThanOrEqualTo(new Date().getTime())
const fulfilled = TestUtils.findRenderedComponentWithType(container, Container)
expect(fulfilled.state.startedAts.testFetch).toEqual(startedAt)
done()
Expand Down Expand Up @@ -801,7 +801,7 @@ describe('React', () => {
})

it('should refresh when refreshInterval is provided', (done) => {
const interval = 100000 // set sufficently far out to not happen during test
const interval = 100000 // set sufficiently far out to not happen during test

@connect(() => ({ testFetch: { url: `/example`, refreshInterval: interval } }))
class Container extends Component {
Expand Down Expand Up @@ -856,6 +856,77 @@ describe('React', () => {
})
})

it('should not set refreshTimeouts when component is unmounted', (done) => {
const interval = 100000 // set sufficiently far out to not happen during test

@connect(() => ({ testFetch: { url: `/example`, refreshInterval: interval } }))
class WithProps extends Component {
render() {
return <Passthrough {...this.props}/>
}
}

class OuterComponent extends Component {
constructor() {
super()
this.state = { render: true }
}

setRender(render) {
this.setState({ render })
}

render() {
if (this.state.render) {
return (
<div>
<WithProps {...this.state} />
</div>
)
} else {
return <div />
}
}
}

const outerComponent = TestUtils.renderIntoDocument(
<OuterComponent />
)

const container = TestUtils.findRenderedComponentWithType(outerComponent, WithProps)

expect(container.state.data.testFetch).toIncludeKeyValues({
fulfilled: false, pending: true, reason: null, refreshing: false, rejected: false, settled: false, value: null
})
expect(container.state.mappings.testFetch.refreshInterval).toEqual(interval)
expect(container.state.refreshTimeouts.testFetch).toEqual(undefined)

setImmediate(() => {
expect(container.state.data.testFetch).toIncludeKeyValues({
fulfilled: true, pending: false, reason: null, refreshing: false, rejected: false, settled: true, value: { T: 't' }
})
expect(container.state.mappings.testFetch.refreshInterval).toEqual(interval)
const refreshTimeout = container.state.refreshTimeouts.testFetch
expect(refreshTimeout).toBeTruthy()
const after = refreshTimeout._onTimeout

// Cancel scheduled refresh
clearTimeout(refreshTimeout)

outerComponent.setRender(false)

// force the refresh to happen now after it has been unmounted
after()
Copy link
Copy Markdown
Contributor

@ryanbrainard ryanbrainard Apr 19, 2017

Choose a reason for hiding this comment

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

can you explain what this is doing? it appears that it's just forcing the call the the refreshTimeout._onTimeout, but doesn't seem like that would actually test the code you added.

Copy link
Copy Markdown
Contributor Author

@aaronschwartz aaronschwartz Apr 19, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I'm forcing the refreshTimeout._onTimeout after the component has been unmounted. If _onTimeout is called without the setRender(false), the setTimeout gets called and the refresh continues indefinitely even though the component has unmounted.

If you remove the code I added, this test does indeed fail. I'm not sure this is the proper fix, but it seems to be working for the issues I've been having.

Copy link
Copy Markdown
Contributor

@ryanbrainard ryanbrainard Apr 19, 2017

Choose a reason for hiding this comment

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

makes sense


const spy = expect.spyOn(window, 'setTimeout')
Copy link
Copy Markdown
Contributor

@ryanbrainard ryanbrainard Apr 19, 2017

Choose a reason for hiding this comment

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

shouldn't this happen before after()? I'm also not really clear what's going on there with the spy.destroy() below..

Copy link
Copy Markdown
Contributor Author

@aaronschwartz aaronschwartz Apr 19, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Hmm.. You're right that it is strange that it is after but I was following the 'should not call setState if component is unmounted' test which does it similarly. But it seems to be working like this because it fails without my change and passes with it.

I was having a hard time coming up with a better test than this. If you have any ideas that would be great.

Copy link
Copy Markdown
Contributor

@ryanbrainard ryanbrainard Apr 20, 2017

Choose a reason for hiding this comment

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

I just tested this (and the other test) with the spy up before after() and without the spy.destroy() and it worked fine. The reason it didn't really matter is that the setImmediate is what triggers the change by yielding to the promise. I'll merge this as is and then fix both of the tests.

setImmediate(() => {
spy.destroy()
expect(spy.calls.length).toBe(0)
done()
})
})
})

it('should not set refreshTimeouts when refreshInterval is not provided', (done) => {
@connect(() => ({ testFetch: `/example` }))
class Container extends Component {
Expand Down