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

fix: update default props to JS default params #1598

Merged
merged 7 commits into from
Sep 26, 2024
Merged

fix: update default props to JS default params #1598

merged 7 commits into from
Sep 26, 2024

Conversation

kabaros
Copy link
Collaborator

@kabaros kabaros commented Sep 14, 2024

Implements LIBS-682

As we move to React 18 in platform as part ofhttps://github.com/dhis2/app-platform/pull/876, all our components and apps need to stop using defaultProps as these were deprecated and started generating a warning since React 18.3.

Most of this transformation was done using a codemod - the codemod is a bit brittle and tied to this project for now, but can be made more generic and incorporated in d2 migrate as we test it in more projects.

@dhis2-bot
Copy link
Contributor

dhis2-bot commented Sep 14, 2024

🚀 Deployed on https://pr-1598--dhis2-ui.netlify.app

@dhis2-bot dhis2-bot temporarily deployed to netlify September 14, 2024 14:25 Inactive
@dhis2-bot dhis2-bot temporarily deployed to netlify September 14, 2024 16:27 Inactive
@dhis2-bot dhis2-bot temporarily deployed to netlify September 14, 2024 17:16 Inactive
@kabaros kabaros marked this pull request as ready for review September 14, 2024 17:24
@kabaros kabaros requested a review from a team as a code owner September 14, 2024 17:24
@kabaros kabaros requested a review from a team September 14, 2024 17:24
@dhis2-bot dhis2-bot temporarily deployed to netlify September 15, 2024 17:50 Inactive
@dhis2-bot dhis2-bot temporarily deployed to netlify September 25, 2024 13:11 Inactive
@KaiVandivier
Copy link
Contributor

KaiVandivier commented Sep 25, 2024

Worked with @amcgee and found that the tests that were hanging were ones using Popper components, and then discovered that the default modifiers = [] param was creating a new array on each rerender, which triggered a useMemo recomputation since it's a dependency, which caused a render loop

Setting the prop default to a stable object reference fixed the loop issue, which was manually noticeable in storybook stories, so we updated other default props to similar stable objects -- running tests now to see if they work 🤞

Update: Select and Popper tests work for me locally now, but I haven't run the whole test suite... cypress caused a stack overflow when I tried to do that 😞

Copy link
Member

@amcgee amcgee left a comment

Choose a reason for hiding this comment

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

With the update to use stable default prop references this looks good to me!

@kabaros
Copy link
Collaborator Author

kabaros commented Sep 25, 2024

great find! thanks @amcgee @KaiVandivier

@dhis2-bot dhis2-bot temporarily deployed to netlify September 25, 2024 19:00 Inactive
@kabaros kabaros merged commit 20e343f into master Sep 26, 2024
16 checks passed
@kabaros kabaros deleted the update-props branch September 26, 2024 08:47
dhis2-bot added a commit that referenced this pull request Sep 26, 2024
## [9.11.5](v9.11.4...v9.11.5) (2024-09-26)

### Bug Fixes

* update default props to JS default params ([#1598](#1598)) ([20e343f](20e343f))
@dhis2-bot
Copy link
Contributor

🎉 This PR is included in version 9.11.5 🎉

The release is available on:

Your semantic-release bot 📦🚀

dhis2-bot added a commit that referenced this pull request Nov 14, 2024
# [10.0.0-alpha.7](v10.0.0-alpha.6...v10.0.0-alpha.7) (2024-11-14)

### Bug Fixes

* **sharing-dialog:** add 'route' to SharingType type ([#1617](#1617)) ([1d9664a](1d9664a))
* revert "fix: select-field not showing in correct place" ([#1614](#1614)) ([c83de6b](c83de6b))
* **translations:** sync translations from transifex (master) ([491bf04](491bf04))
* update default props to JS default params ([#1598](#1598)) ([20e343f](20e343f))

### Features

* merge branch master into branch ([0577074](0577074))
* **icons:** add link-off icons ([e72be1e](e72be1e))
@dhis2-bot
Copy link
Contributor

🎉 This PR is included in version 10.0.0-alpha.7 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

4 participants