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

Fixes needless custom component recreation (#1195) #1224

Merged
merged 4 commits into from
Oct 29, 2024

Conversation

shawncrawley
Copy link
Contributor

@shawncrawley shawncrawley commented Jul 18, 2024

As detailed in issue #1195, custom components were being needlessly re-created on every re-render. This was due to the vdomImportSource (i.e. model.importSource) of the custom component being used as a dependency of the useEffect function that creates/binds the custom components. Using a JavaScript object (i.e. dictionary) as a dependency was an issue in this case since the object in question was being recreated anew every re-render even though its actual key/value entries were not.

I initially completely removed vdomImportSource from the dependencies, not understanding a use case where it would ever change anyway, but second guessed its importance due to it being there in the first place. I ended up finding this article and opted for their recommended "Approach 4", which is to JSON.stringify the object and use that as the dependency instead.

I also went ahead and refactored the react.js web template file to use the new createRoot(...).render() workflow in ReactJS 18, as mentioned by @rmorshea in the original issue. Although it seems to be working great for me, I'm definitely not a ReactPy nor even a ReactJS expert, so that should definitely be looked over by someone more experienced.

Checklist

Please update this checklist as you complete each item:

  • Tests have been developed for bug fixes or new functionality.
  • The changelog has been updated, if necessary.
  • Documentation has been updated, if necessary.
  • GitHub Issues closed by this PR have been linked.

By submitting this pull request I agree that all contributions comply with this project's open source license(s).

@shawncrawley
Copy link
Contributor Author

I'm not sure on those checklist items above,. It seemed at least a few wouldn't be relevant here, but let me know if I'm wrong there and I'm happy to wrap them up.

@Archmonger
Copy link
Contributor

Tests are failing, due to a mixture of unpinned test dependencies and using old/deprecated github workflows for NodeJS setup.

You are free to do some quick fixes to get the tests functioning, or wait for me to fix them in a different PR. Hopefully I'll have time in the next week or so.

@Archmonger Archmonger linked an issue Oct 29, 2024 that may be closed by this pull request
@Archmonger
Copy link
Contributor

cc: @williamneto

@Archmonger Archmonger merged commit 0e4432f into reactive-python:main Oct 29, 2024
17 checks passed
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.

Client-side component events break input focus
2 participants