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

Fix some react warnings in GroupMemberList #1522

Merged
merged 2 commits into from
Oct 24, 2017
Merged

Conversation

lukebarnard1
Copy link
Contributor

  • If the list contains two users twice, react would warn about duplicate keys. Use index instead.
  • Check if unmounted before setting state after fetching members.

 - If the list contains two users twice, react would warn about duplicate keys. Use `index` instead.
 - Check if unmounted before setting state after fetching members.
Copy link
Member

@dbkr dbkr left a comment

Choose a reason for hiding this comment

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

Hmm, using list index as a key is a surefire way to confuse React and React doesn't like you doing it (because it gives it no more information than it would have otherwise). Why would we ever have duplicate users in the list?

@lukebarnard1
Copy link
Contributor Author

Hmm, using list index as a key is a surefire way to confuse React and React doesn't like you doing it (because it gives it no more information than it would have otherwise)

Ah. I had no idea about this but that makes a lot of sense.

Why would we ever have duplicate users in the list?

The API allows it (currently). E.g. Amandine is +matrix twice.

@dbkr
Copy link
Member

dbkr commented Oct 24, 2017

It's presumably not meaningful for the same user to be in the list twice though? I would confer with Erik, but I think I would vote for ignoring the dupes until synapse gets fixed to not send dupes?

@lukebarnard1
Copy link
Contributor Author

ignoring the dupes until synapse gets fixed to not send dupes?

This is fair. And then userId would be an OK key.

Copy link
Member

@dbkr dbkr left a comment

Choose a reason for hiding this comment

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

ES6 has Set() ftr :)

@dbkr dbkr merged commit ba46faf into develop Oct 24, 2017
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.

2 participants