-
Notifications
You must be signed in to change notification settings - Fork 33
refactor: [DHIS2-18801] upgrade capture app to use vite and react 18 #4375
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
base: master
Are you sure you want to change the base?
Conversation
…nent & use it in NoSelectionsInfoBox
|
🚀 Deployed on https://deploy-preview-4375.capture.netlify.dhis2.org |
…le to enforce this convention
@henrikmv Thank you for your review! I’ve now removed and refactored most of the deprecated Let me know if you have any additional feedback. Thanks! |
eirikhaugstulen
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @simonadomnisoru - really great work on this! 🎉
Left some small comments, mostly NITs, but have a look and let me know if you disagree!
|
|
||
| const allEnvVariables = { | ||
| ...envFromCypressFiles, | ||
| ...(process.env || {}), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haven't tried to run this myself, so it might work - but should this be import.meta.env instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
process.env still works correctly and Cypress runs successfully both locally and in GitHub workflows. Feel free to try it in your local environment as well if you’d like to confirm. Thanks!
...tries/common/TEIAndEnrollment/useMetadataForRegistrationForm/hooks/useDataEntryFormConfig.ts
Outdated
Show resolved
Hide resolved
src/core_modules/capture-core/components/FormFields/withInternalChangeHandler.tsx
Outdated
Show resolved
Hide resolved
.../components/PossibleDuplicatesDialog/ReviewDialogContents/ReviewDialogContents.component.tsx
Show resolved
Hide resolved
| @@ -17,7 +17,7 @@ const computeServerCacheVersion = ({ minor, patch, tag }) => | |||
| computeTagVersionPart(tag); | |||
|
|
|||
| const computeAppCacheVersion = () => { | |||
| const appCacheVersionAsString = process.env.REACT_APP_CACHE_VERSION; | |||
| const appCacheVersionAsString = process.env.DHIS2_CACHE_VERSION; | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be import.meta.env.DHIS2_CACHE_VERSION or is the app-shell doing some magic here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The app-shell is doing some magic, and according to the migration guide, process.env is well supported and a more widely-used pattern. Refactoring to import.meta.env would require extra effort, as Jest doesn’t support it by default. Therefore, continuing to use process.env seems reasonable to me.
eirikhaugstulen
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work @simonadomnisoru - this is a big improvement!
henrikmv
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Brilliant work!
JoakimSM
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks quite good! Let me spend some more time on this tomorrow, just wanted to give some initial comments.
| const rawStyles = stylesOrCreator && typeof stylesOrCreator === 'function' | ||
| ? stylesOrCreator(theme as T) | ||
| : stylesOrCreator; | ||
|
|
||
| // Transform the raw style object with Emotion’s css() so that className={classes.label} continues to work as before | ||
| const classes = Object.keys(rawStyles).reduce((acc, key) => { | ||
| acc[key] = css(rawStyles[key] as any); | ||
| return acc; | ||
| }, {} as any); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
Given we have this code in a "centralized" place, maybe we can wrap in useMemo for a minor perf. improvement?
| const rawStyles = stylesOrCreator && typeof stylesOrCreator === 'function' | |
| ? stylesOrCreator(theme as T) | |
| : stylesOrCreator; | |
| // Transform the raw style object with Emotion’s css() so that className={classes.label} continues to work as before | |
| const classes = Object.keys(rawStyles).reduce((acc, key) => { | |
| acc[key] = css(rawStyles[key] as any); | |
| return acc; | |
| }, {} as any); | |
| const classes = useMemo(() => { | |
| const rawStyles = stylesOrCreator && typeof stylesOrCreator === 'function' | |
| ? stylesOrCreator(theme as T) | |
| : stylesOrCreator; | |
| // Transform the raw style object with Emotion’s css() so that className={classes.label} continues to work as before | |
| return Object.keys(rawStyles).reduce((acc, key) => { | |
| acc[key] = css(rawStyles[key] as any); | |
| return acc; | |
| }, {} as any); | |
| }, []); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great suggestion! I added it, thanks.
| "workspaces": [ | ||
| "packages/rules-engine" | ||
| ], | ||
| "dependencies": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general, I do think we should explicitly list the dependencies we use directly. if not, the depedency graph becomes ambiguous. Not sure we should make an exception here? I understand we will have to make sure we use the same version as app-runtime, but at least another dep update won't suddenly change the version we are using.
src/components/AppLoader/init.ts
Outdated
|
|
||
| return new Promise<void>((resolve) => { | ||
| import(`moment/locale/${locale}`) | ||
| import(`moment/dist/locale/${locale}`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looked at your solution for dateFns and it seems like we have a problem with moment as well. The moment locale is not being properly set from what I can see (have a look at the weekdays variable in the setLocaleDataAsync function in this file, still resolves to english even if the ui-language is something else).
These dynamic imports are quite confusing, not going to lie. Tried the import.meta.glob thing a bit, wasn't able to make it work. Let's talk about this tomorrow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@JoakimSM I’ve removed the dynamic import and use the switch statement for moment as well. Could you please take another look? Thanks!
| </MuiThemeProvider> | ||
| </JSSProviderShell> | ||
| </React.Fragment> | ||
| <QueryClientProvider client={queryClient}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really need this? I think this is already inserted by the App shell / runtime?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Had a look and the problem is that we are adding multiple versions of react-query to the bundle and the app and app-runtime are using different versions (if you have a look in yarn.lock, you will see react-query listed twice). I fixed it by doing this:
yarn add @tanstack/react-query@^4.42.0 -W
npx yarn-deduplicate --packages @tanstack/react-query
you should end up with something like this in yarn.lock:
"@tanstack/react-query@^4.42.0", "@tanstack/react-query@^4.40.0":
version "4.42.0"
resolved "https://registry.yarnpkg.com/@tanstack/react-query/-/react-query-4.42.0.tgz#a4d2527713e841c71a4a4b3f9412bb98ea5eec59"
integrity sha512-j0tiofkzE3CSrYKmVRaKuwGgvCE+P2OOEDlhmfjeZf5ufcuFHwYwwgw3j08n4WYPVZ+OpsHblcFYezhKA3jDwg==
dependencies:
"@tanstack/query-core" "4.41.0"
use-sync-external-store "^1.2.0"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Amazing, yes running the deduplicate fixed the issue. Thanks for the suggestion! 🙌
| export type WithStyles<S extends Record<string, any> | ((t: typeof theme) => S)> = { | ||
| classes: Record<string, any>; | ||
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like we have a job to do on the types in this file, ok to leave that for later though. But can we change the WithStyles to the following suggestion:
| export type WithStyles<S extends Record<string, any> | ((t: typeof theme) => S)> = { | |
| classes: Record<string, any>; | |
| }; | |
| type Style = Record<string, string | number> | |
| export type WithStyles<T extends Record<string, Style> | ((t: typeof theme) => Record<string, Style>)> = { | |
| classes: { [K in keyof (T extends (args: unknown) => unknown ? ReturnType<T> : T)]: string } | |
| } |
Let me know if something breaks, but I think it should be an improvement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using the suggested types causes errors because string | number is too restrictive and doesn’t account for the media queries or the pseudo-selectors (e.g. &:focus, &:hover, &.isLastItem etc.). To fix the errors, I extended on your suggestion so that type Style also supports nested objects. Let me know if you agree. Thank you!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perfect, that makes sense 👍
|





DHIS2-18801
This pull request migrates the project from Create React App to Vite, following the official migration guide
Key changes introduced during the migration:
@dhis2/cli-app-scripts,@dhis2/uiand@dhis2/app-runtimecreateRoot(e.g. DeleteControl.component.tsx)classnameslibrary and replaced it with Emotion’s cx helper.isInitialLoadingfor disabled queries.yarn build:standaloneandyarn serve), useful for running Cypress tests or inspecting the production bundle.