Skip to content
This repository was archived by the owner on Sep 11, 2024. It is now read-only.

Fix some races due to promises completing after we've switched rooms#84

Merged
richvdh merged 2 commits into
developfrom
rav/fix_refs_npes
Jan 11, 2016
Merged

Fix some races due to promises completing after we've switched rooms#84
richvdh merged 2 commits into
developfrom
rav/fix_refs_npes

Conversation

@richvdh
Copy link
Copy Markdown
Member

@richvdh richvdh commented Jan 8, 2016

Add a few isMounted() checks to promise handlers so that we don't end up
throwing NPEs.

This should fix element-hq/element-web#589.

Add a few isMounted() checks to promise handlers so that we don't end up
throwing NPEs.
Comment thread src/components/structures/RoomView.js Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This probably needs to be self?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

apparently. Evidently this was super-well-tested :(

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

done

@kegsay
Copy link
Copy Markdown
Member

kegsay commented Jan 11, 2016

LGTM aside from self/this thing.

It is worth noting that in their infinite wisdom, Facebook are planning on deprecating isMounted() because they deem it to be an anti-pattern. They argue that all async operations should be shot in the head at the moment of componentWillUnmount. This is easy to do for callbacks (unregister the listener!). This is not so easy with promises/coroutines. Their suggested "right" way is horrible : monkey-patch the Promise class to include a cancel() method. Cancelling promises sensibly remains an open problem, so the idea of doing this leaves a bad taste in my mouth. Either way, this is what they've decided to do.

This means that in a later version of React, this code will be broken. We can either pre-emptively catch this (and have our own isMounted() check with a boolean marked on componentWillMount and componentWillUnmount), do the promise monkey patch, or leave it as is. I'd opt for #1 or #3 with a comment explaining the problem.

@richvdh
Copy link
Copy Markdown
Member Author

richvdh commented Jan 11, 2016

ugh. Tiresome.

@richvdh
Copy link
Copy Markdown
Member Author

richvdh commented Jan 11, 2016

@kegsay: ptal

@kegsay
Copy link
Copy Markdown
Member

kegsay commented Jan 11, 2016

LGTM

richvdh added a commit that referenced this pull request Jan 11, 2016
Fix some races due to promises completing after we've switched rooms
@richvdh richvdh merged commit e7740cb into develop Jan 11, 2016
@richvdh richvdh deleted the rav/fix_refs_npes branch January 11, 2016 15:25
dtygel pushed a commit to coletivoEITA/matrix-react-sdk that referenced this pull request May 15, 2017
…matrix-react-sdk-strings

Update from Weblate.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

this.refs.geminiPanel can sometimes return null and crash everything

2 participants