Skip to content
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.

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

Prevent the context from changing which forces unnecessary rerenders #2373

Merged
merged 1 commit into from May 3, 2023

Conversation

mdkwock
Copy link
Contributor

@mdkwock mdkwock commented Apr 13, 2023

Fixes #2370 by saving the context as a state object so the object doesn't change. Currently whenever the GiftedChat component re-renders it causes the context to change as the context is given a new value. this causes all the components that rely on the context to update, which is a lot of them.

@mdkwock
Copy link
Contributor Author

mdkwock commented Apr 13, 2023

One note is I'm not entirely sure what other bugs this may bring out that were being hidden by the constant re-renders

@amerikan
Copy link
Contributor

@Johan-dutoit looks good to me. This seems to fix some lag!

@Eigilak
Copy link

Eigilak commented Apr 26, 2023

Looks good to me!

@hstaudacher
Copy link

@Johan-dutoit would you mind taking a look at this?

@Johan-dutoit
Copy link
Collaborator

Defo want to merge this in. Please resolve conflicts 🙏

@amerikan
Copy link
Contributor

amerikan commented May 2, 2023

@mdkwock please resolve conflicts if you can. If not, I can try soon! @Johan-dutoit this is a much needed change since this makes everything work a lot faster! 🐎 Kudos to @mdkwock for tackling this.

@amerikan
Copy link
Contributor

amerikan commented May 2, 2023

@mdkwock @Johan-dutoit i ported over this to the new hook based component: #2379

@mdkwock
Copy link
Contributor Author

mdkwock commented May 3, 2023

@amerikan i would probably rewrite what i wrote to not use state, and instead use useMemo with dependencies. this would be a bit cleaner and keep the values out of state. I couldn't do this originally because as a class component you don't have access to the useMemo hooks and the like.

@mdkwock mdkwock reopened this May 3, 2023
@amerikan
Copy link
Contributor

amerikan commented May 3, 2023

@mdkwock thanks for the feedback! The other PR is one of my many planned iterations to modernize the main component now that's using functional components with hooks. I just threw in your modifications as a separate commit so that the library is useable as soon as possible.

@mdkwock
Copy link
Contributor Author

mdkwock commented May 3, 2023

@amerikan @Johan-dutoit okay resolved conflicts and updated to use useMemo as my comment stated, i think it's cleaner looking than using state.

@amerikan
Copy link
Contributor

amerikan commented May 3, 2023

@mdkwock nice! I just removed the commit from my PR since you're addressing it here.

@Johan-dutoit when you merge this, it'll be good to bump version on npm 🙏🏼

@Johan-dutoit Johan-dutoit merged commit 2beba0a into FaridSafi:master May 3, 2023
@Johan-dutoit
Copy link
Collaborator

@amerikan FYI bumping npm is unavailable until the typescript/eslint errors are resolved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Excessive rerenders because of context changes
5 participants