-
Notifications
You must be signed in to change notification settings - Fork 112
(fix)O3-1144: Remove redundant type definitions, use types from form-engine #998
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: main
Are you sure you want to change the base?
Conversation
…e-lib - Import and re-export types (FormField, FormPage, FormSection, FormQuestionOptions, etc.) from @openmrs/esm-form-engine-lib instead of defining them locally - Create type aliases (Page, Section, Question, QuestionOptions) pointing to form-engine types for backward compatibility - Export additional useful types (QuestionAnswerOption, HideProps, DisableProps, RepeatOptions, etc.) from form-engine-lib - Update add-form-reference.modal.tsx to use FormPage, FormSection, FormField instead of local Page, Section, Question types - All tests passing, no breaking changes
|
@NethmiRodrigo just got drained out in this issue...please let me know if these changes works |
NethmiRodrigo
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.
Thanks @RajPrakash681
src/components/interactive-builder/modals/add-form-reference/add-form-reference.modal.tsx
Outdated
Show resolved
Hide resolved
| translations?: Record<string, string>; | ||
| } | ||
| // Using extended FormBuilderSchema instead of FormSchema | ||
| export type Schema = FormBuilderSchema; |
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.
We should just directly import all types from the form-engine-lib itself and remove them from this file, we shouldn't need to export from here. Even though that would mean that we need to update a lot of files, it just seems redundant to export them from here again
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.
We should just directly import all types from the form-engine-lib itself and remove them from this file, we shouldn't need to export from here. Even though that would mean that we need to update a lot of files, it just seems redundant to export them from here again
You're absolutely right! Re-exporting these types is redundant. I'll update the codebase to import types directly from @openmrs/openmrs-form-engine-lib where they're used, rather than going through this intermediary file. This will make the dependencies clearer and reduce unnecessary abstraction.
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.
Why are we still re-exporting the schema from here? If it’s because the type from form-engine-lib is missing the description property, it’s highly possible that it's a mistake. Could you ask this in the #form-engine channel on Slack, and fix it in the form engine?
- Remove re-exported types from src/types.ts - Update all files to import FormField, FormPage, FormSection directly from @openmrs/esm-form-engine-lib - Keep only local types in src/types.ts (FormBuilderSchema, Form, Schema, Concept, etc.) - This eliminates redundant type re-exports and makes dependencies clearer
239de4d to
ab20c96
Compare
|
@NethmiRodrigo good morning ma'am any updates about it..? |
oh woow!!! 😊 |
|
@NethmiRodrigo i think this issue's requirement is fulfilled please have a look and tell what more changes it needs!! |
|
@NethmiRodrigo this one also |
| translations?: Record<string, string>; | ||
| } | ||
| // Using extended FormBuilderSchema instead of FormSchema | ||
| export type Schema = FormBuilderSchema; |
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.
Why are we still re-exporting the schema from here? If it’s because the type from form-engine-lib is missing the description property, it’s highly possible that it's a mistake. Could you ask this in the #form-engine channel on Slack, and fix it in the form engine?
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.
Can you please undo the changes to this file? And change your end of line as was mentioned in one of your other PRs in a different repo?
|
Also, I believe I’ve mentioned this before, please don’t change the PR template. Whenever you create a new PR, there will be a template, which you can fill out. Please stick to that and do not change it. AI tends to change it all the time, so make sure that you double check and change it back, this applies to all repositories, except where there isn’t a template. |
Thanks for pointing that out, Nethmi. Usually, I fill out the PR template manually after you guided me about it earlier, but I missed it this time since I was in a bit of a hurry. Sorry about that I’ll make sure to double-check and stick to the template in all future PRs |
2ec0f43 to
d0be6e2
Compare
I'll ask about this in the #form-engine Slack channel and make the necessary changes based on their guidance. Will update you once I hear back. |
The real question is whether or not the form-engine-lib needs to know about the description property on the FormSchema and I'm not entirely certain what it would do with that information. Thus, the thing the form-builder (which does use the description) is doing is probably right for now.
|
|
@NethmiRodrigo hello, hope you would be doing well ! what should i do now in this.. any update ? |
NethmiRodrigo
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.
One minor nit
src/components/interactive-builder/modals/add-form-reference/add-form-reference.modal.tsx
Outdated
Show resolved
Hide resolved
…dd-form-reference.modal.tsx Co-authored-by: Nethmi Rodrigo <[email protected]>
Done! |
|
@NethmiRodrigo Hi, can this be merged now or does this require more things to fix!? |
Requirements
Summary
This PR removes redundant type definitions from the form builder and uses types directly from
@openmrs/esm-form-engine-lib.Changes made:
Schema,Page,Section,Question, andQuestionOptions@openmrs/esm-form-engine-libFormBuilderSchemainterface that extendsFormSchemato include thedescriptionproperty, which is specific to the form builder's needssrc/types.tsto exportFormBuilderSchemaasSchemafor backward compatibilityScreenshots
N/A - No UI changes
Related Issue
https://issues.openmrs.org/browse/O3-1144
Other
None