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) App not hot updating with slot config changes #357

Merged
merged 3 commits into from
Mar 15, 2022

Conversation

brandones
Copy link
Collaborator

@brandones brandones commented Mar 14, 2022

Requirements

  • This PR has a title that briefly describes the work done, including the ticket number if there is a ticket.

For changes to apps

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

Fixes a bug where the app does not hot update in response to extension slot config changes.

Includes a breaking change to useStore and useStoreState, but this will not affect any existing users.

Screenshots

remove-ext

@brandones brandones changed the title App not hot updating with slot config changes (fix) App not hot updating with slot config changes Mar 14, 2022
@github-actions
Copy link
Contributor

File size impact

Merging ext-slot-conf-update into master impact files as follow:

@openmrs/esm-devtools-app (+1.81%)
Files new size
packages/apps/esm-devtools-app/dist/217.js 411 kB (+7.75 kB / +1.92%) ↗️
Unmodified (4) 25.2 kB (0 B / +0%) 👻
Total (5) 436 kB (+7.75 kB / +1.81%) ↗️
@openmrs/esm-implementer-tools-app (+0.33%)
Files new size
packages/apps/esm-implementer-tools-app/dist/217.js 411 kB (+7.75 kB / +1.92%) ↗️
packages/apps/esm-implementer-tools-app/dist/371.js 216 kB (+38 B / +0.02%) ↗️
packages/apps/esm-implementer-tools-app/dist/494.js 7 kB (+27 B / +0.39%) ↗️
Unmodified (18) 1.72 MB (0 B / +0%) 👻
Total (21) 2.35 MB (+7.81 kB / +0.33%) ↗️
@openmrs/esm-login-app (+0.39%)
Files new size
packages/apps/esm-login-app/dist/217.js 411 kB (+7.75 kB / +1.92%) ↗️
Unmodified (27) 1.61 MB (0 B / +0%) 👻
Total (28) 2.02 MB (+7.75 kB / +0.39%) ↗️
@openmrs/esm-offline-tools-app (+0.28%)
Files new size
packages/apps/esm-offline-tools-app/dist/217.js 411 kB (+7.75 kB / +1.92%) ↗️
Unmodified (25) 2.37 MB (0 B / +0%) 👻
Total (26) 2.78 MB (+7.75 kB / +0.28%) ↗️
@openmrs/esm-primary-navigation-app (+0.36%)
Files new size
packages/apps/esm-primary-navigation-app/dist/217.js 411 kB (+7.75 kB / +1.92%) ↗️
Unmodified (17) 1.74 MB (0 B / +0%) 👻
Total (18) 2.15 MB (+7.75 kB / +0.36%) ↗️
@openmrs/esm-app-shell (+0.17%)
Files new size
packages/shell/esm-app-shell/dist/openmrs.js 1.42 MB (+3.63 kB / +0.26%) ↗️
packages/shell/esm-app-shell/dist/service-worker.js 164 kB (0 B / +0%) 👻
Unmodified (2) 538 kB (0 B / +0%) 👻
Total (4) 2.12 MB (+3.63 kB / +0.17%) ↗️
Generated by @jsenv/file-size-impact during Report bundle size#1983110121 on 78fef85

@github-actions
Copy link
Contributor

File size impact

Merging ext-slot-conf-update into master impact files as follow:

@openmrs/esm-devtools-app (+1.81%)
Files new size
packages/apps/esm-devtools-app/dist/217.js 411 kB (+7.75 kB / +1.92%) ↗️
Unmodified (4) 25.2 kB (0 B / +0%) 👻
Total (5) 436 kB (+7.75 kB / +1.81%) ↗️
@openmrs/esm-implementer-tools-app (+0.33%)
Files new size
packages/apps/esm-implementer-tools-app/dist/217.js 411 kB (+7.75 kB / +1.92%) ↗️
packages/apps/esm-implementer-tools-app/dist/371.js 216 kB (+38 B / +0.02%) ↗️
packages/apps/esm-implementer-tools-app/dist/494.js 7 kB (+27 B / +0.39%) ↗️
Unmodified (18) 1.72 MB (0 B / +0%) 👻
Total (21) 2.35 MB (+7.81 kB / +0.33%) ↗️
@openmrs/esm-login-app (+0.39%)
Files new size
packages/apps/esm-login-app/dist/217.js 411 kB (+7.75 kB / +1.92%) ↗️
Unmodified (27) 1.61 MB (0 B / +0%) 👻
Total (28) 2.02 MB (+7.75 kB / +0.39%) ↗️
@openmrs/esm-offline-tools-app (+0.28%)
Files new size
packages/apps/esm-offline-tools-app/dist/217.js 411 kB (+7.75 kB / +1.92%) ↗️
Unmodified (25) 2.37 MB (0 B / +0%) 👻
Total (26) 2.78 MB (+7.75 kB / +0.28%) ↗️
@openmrs/esm-primary-navigation-app (+0.36%)
Files new size
packages/apps/esm-primary-navigation-app/dist/217.js 411 kB (+7.75 kB / +1.92%) ↗️
Unmodified (17) 1.74 MB (0 B / +0%) 👻
Total (18) 2.15 MB (+7.75 kB / +0.36%) ↗️
@openmrs/esm-app-shell (+0.17%)
Files new size
packages/shell/esm-app-shell/dist/openmrs.js 1.42 MB (+3.63 kB / +0.26%) ↗️
packages/shell/esm-app-shell/dist/service-worker.js 164 kB (0 B / +0%) 👻
Unmodified (2) 538 kB (0 B / +0%) 👻
Total (4) 2.12 MB (+3.63 kB / +0.17%) ↗️
Generated by @jsenv/file-size-impact during Report bundle size#1983150989 on d5ae830

);
const slotStore = getExtensionSlotsConfigStore();
const oldState = slotStore.getState();
const newState = { slots: { ...oldState.slots, ...newSlotStoreEntries } };
Copy link
Contributor

Choose a reason for hiding this comment

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

This will not remove oldState slots if they are meant to be removed. Is it desired to remove old unused slots from state?

Copy link
Member

Choose a reason for hiding this comment

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

I asked @brandones a similar question once and he indicated the idea is to keep the slots around so that the implementer tools can track them even if the slot itself is no longer rendered.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@ibacher is correct. There will not be so many that it will be a memory problem.


function useStore<T, U>(store: Store<T>): T;
function useStore<T, U>(store: Store<T>, select: (state: T) => U): U;
function useStore<T, U>(
Copy link
Contributor

Choose a reason for hiding this comment

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

What are all of these declarations for? You're just declaring a function then immediately overwriting it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@brandones brandones requested a review from ZacButko March 15, 2022 15:04
@brandones brandones merged commit 80572e7 into master Mar 15, 2022
@brandones brandones deleted the ext-slot-conf-update branch March 15, 2022 16:25
@ZacButko ZacButko mentioned this pull request May 3, 2022
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.

3 participants