(fix) O3-4080: Option of adding a key in workspace

[kb019]

Requirements

• [] This PR has a title that briefly describes the work done including the ticket number. Ensure your PR title includes a conventional commit label (such as feat, fix, or chore, among others). See existing PR titles for inspiration.

For changes to apps

• My work conforms to the O3 Styleguide and design documentation.

If applicable

• My work includes tests or is validated by existing tests.
• I have updated the esm-framework mock to reflect any API changes I have made.

Summary

When we have workspaces where titles are set dynamically and trying to launch workspaces of same name back to back does not cause for the title to update. You can see one such use case here . The reason is we use workspace name as the key here which causes the parcel component to not unmount and to restrict calling useEffect only for the first time when workspace of a particular name is launched.This in turn does not cause the title to update . By passing unique key we make sure that the useEffect is triggered each time the workspace is called. I will paste some videos below to get some more understanding. I used implementer tools in esm core for easier debugging. I have made the key as optional in order to not break existing code.

In the video below you can see that the only the content of workspace changes when clicked on dependency name but not the title
OpenMRS - Personal - Microsoft_ Edge 2024-10-10 22-46-26 (1).webm

In the below video i am adding a key prop while calling launch workspace
backend-dependencies.component.tsx - openmrs-esm-core - Visual Studio Code 2024-10-10 22-47-22 (1).webm

In the Video below after adding the key prop, you can see that both the title and the content are changed
OpenMRS - Personal - Microsoft_ Edge 2024-10-10 22-48-51 (1).webm

Also i think that the additional props gets overridden when calling workspace of same name second time. I have made the necessary changes to not overide additionalProps.

Screenshots

Related Issue

https://openmrs.atlassian.net/browse/O3-4080

Other

[usamaidrsk]

Thanks @kb019 ! LGTM

[usamaidrsk]

Nice work @kb019 , just a minor suggestion
Thanks

[brandones]

Why don't you just add patient to the useEffect dependency array?

[brandones]

Oh, looks like that was done here: openmrs/openmrs-esm-patient-management#1341 . Which is wonderful! I think that's the right way to solve this problem. Omitting dependencies of a useEffect hook from the dependency array is an error.

[kb019]

Oh, looks like that was done here: openmrs/openmrs-esm-patient-management#1341 . Which is wonderful! I think that's the right way to solve this problem. Omitting dependencies of a useEffect hook from the dependency array is an error.

Hi @brandones . I agree on this. Thanks for the review.But what about the additional props, should we pass it each time the workspace is called. Beacuse currently if we call the workspace on second call without props it does not work. We had this issue in ward app and the workaround we did is here .Is this the correct way?.Do we need to pass props every time while making a call to workspace.Woudn't it be nice if we just pass the props during the first call and have it used in the subsequent calls to the workspace of same name. Thanks.

[brandones]

For posterity: @kb019 messaged me on Slack to let me know he had looked into this further and decided it was not actually an issue. @kb019 it would be great if you could comment to briefly explain why.

[kb019]

Hi all,

The key point is how do we want the workspace to work when called consecutively, do we just want to change the state
or do we want to have the component mounted.

By this i mean, consider you called a workspace like below
launchWorkspace('sampleWorkspace');
and again you called the same workspace consecutively ( i.e launchWorkspace('sampleWorkspace')) how to do want it to work.

Do we want to change the state or remount the component.I think here we are going with a state change which makes sense. For example consible below component

function ParentComponent({workspace}){
    
   return <div>
                 <SubComponent state={workspace} key={workspace.name}/> #Treat as workspace component
         </div>
}

We only want to mount when there is a new workspace called (which is done by having workspace.name as key), and just change the state if you are already calling the workspace which is opened.

The problem i think with this pr is, it goes against this analogy and just mounts whichever workspace you called and that's the reason i felt this was unnecessary.
Thanks.

@brandones , This is just my thought process and i think you have more expertise in this . Please correct if i am wrong .Thanks

[ibacher]

Thanks @kb019 for the clear explanation. I haven't looked into this too deeply, but my gut instinct is that a state change rather than remounting the component will be more likely to result in the "right" behaviour from the end-user perspective, which is ultimately what we care about.

Re: the ward app thing, the cleaner implementation would seem to be to have the ward patient passed as a prop (or context) into the WardPatientActionButton itself, which means probably making it part of the state passed to the action-menu-ward-patient-items-slot slot (it isn't obvious to me where this slot is actually used).

[kb019]

Hi @ibacher , i think for now whatever additional props you pass in the launchWorkspace ,it will be passed to all the workspaces as props which have same side-bar family type . I think we dont differentiate between props which are only required for workspace and props which are only reequired for workspaces which have same side-bar family type . We just put all in additional workspaces both which are required by the component and the one which is shared by all workspaces having same side-bar family type . I think it would be more clear if we hadlaunchWorkspace(name,additionalProps,sideBarProps)rather than only launchWorkspace(name,additionalProps).

Hi @brandones , would like to hear your thoughts on this. Thanks

[brandones]

We just put all in additional workspaces both which are required by the component and the one which is shared by all workspaces having same side-bar family type .

I don't really understand what you wrote, so it's not clear to me what's confusing about the current behavior. And I don't think we should complicate the API without a very good reason.