Slice 1: Initial ModInstance type and adapter applied to re-/activation code (1/3)
#9182
Conversation
| @@ -890,115 +883,3 @@ describe("buildNewMod", () => { | |||
| }, | |||
| ); | |||
| }); | |||
|
|
|||
| describe("findMaxIntegrationDependencyApiVersion", () => { | |||
There was a problem hiding this comment.
Moved to utility folder test file
twschiller
changed the title
Playwright test results
Details
Flaky testsmsedge-setup › setup/affiliated.setup.ts › authenticate with affiliated user Skipped testschrome › tests/regressions/doNotCloseSidebarOnPageEditorSave.spec.ts › #8104: Do not automatically close the sidebar when saving in the Page Editor |
twschiller
changed the title
twschiller
changed the title
| const hasPersonalDeployment = activatedModComponentsForMod?.some( | ||
| (x) => x._deployment?.isPersonalDeployment, | ||
| ); | ||
| const hasPersonalDeployment = |
There was a problem hiding this comment.
Example of simplifying personal deployment code
| * In the future, we might consider eliminating by using a predictable id based on the mod instance id and position | ||
| * in the mod definition. But that's not possible today because the ids use a UUID format. | ||
| */ | ||
| modComponentIds: UUID[]; |
There was a problem hiding this comment.
A key insight I didn't originally anticipate is that ModInstance has to track the mod component ids. (See comment in code for why)
twschiller
changed the title
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #9182 +/- ##
==========================================
+ Coverage 74.24% 74.82% +0.57%
==========================================
Files 1332 1369 +37
Lines 40817 42205 +1388
Branches 7634 7885 +251
==========================================
+ Hits 30306 31580 +1274
- Misses 10511 10625 +114 ☔ View full report in Codecov by Sentry. |
twschiller
changed the title
twschiller
changed the title
| * @since 1.7.37 | ||
| * @note This function is just for safety, there's currently no way for a mod to end up with "mixed" integration api versions. | ||
| */ | ||
| export function findMaxIntegrationDependencyApiVersion( |
There was a problem hiding this comment.
Moved the methods out of page editor directory to where they can be used cross-context
| /** | ||
| * Returns a sharing object private package defined in the user's scope. | ||
| */ | ||
| export function createPrivateSharing(): Sharing { |
There was a problem hiding this comment.
There's a sharing factory. But in general, I think we want to keep prod data creation separate from test factories to avoid dummy data accidentally finding its way into production code
It would be good to align on naming conventions to distinguish, e.g., using create prefix in prod code vs. factory suffix
| @@ -62,7 +62,6 @@ function setupInputs(): { | |||
| modDefinition: ModDefinition; | |||
| } { | |||
| const formValues: WizardValues = { | |||
| modComponents: { 0: true }, | |||
There was a problem hiding this comment.
Dropped unused modComponents toggle field from the type
| export type UseActivateModWizardResult = { | ||
| wizardSteps: WizardStep[]; | ||
| initialValues: WizardValues; | ||
| validationSchema: Yup.AnyObjectSchema; | ||
| }; | ||
|
|
||
| function getInitialIntegrationDependencies( |
There was a problem hiding this comment.
Extracted these 2 from other method because they're both quite long
| @@ -60,8 +60,6 @@ function useModNotFoundRedirectEffect(error: unknown): void { | |||
|
|
|||
| /** | |||
| * Common page for activating a mod definition | |||
| * | |||
| * @param modDefinitionQuery The mod definition to activate | |||
There was a problem hiding this comment.
Invalid documentation
| * Maps activated mod components to a mod instance. | ||
| * @param modComponents mod components from the mod | ||
| */ | ||
| export function mapActivatedModComponentsToModInstance( |
There was a problem hiding this comment.
The primary adapter to review in this PR
| // definition vs. looking up the definition | ||
| options: emptyModOptionsDefinitionFactory(), | ||
| sharing: modMetadata.sharing ?? createPrivateSharing(), | ||
| updated_at: modMetadata.updated_at ?? firstComponent.updateTimestamp, |
There was a problem hiding this comment.
We might consider this line of refactoring as an opportunity to fix the snake case and/or perform other reshaping
There was a problem hiding this comment.
I would perform that at the service layer level. We also want to map extensionPoints -> starterBricks at the same time
twschiller
changed the title
ModInstance type and adapter applied to re-/activation code (1/3)
twschiller
changed the title
ModInstance type and adapter applied to re-/activation code (1/3)ModInstance type and adapter applied to re-/activation code (1/3)
| if (!Array.isArray(options.activatedModComponents)) { | ||
| console.warn("state migration has not been applied yet", { | ||
| options, | ||
| }); | ||
| throw new TypeError("state migration has not been applied yet"); | ||
| } |
There was a problem hiding this comment.
NIT: better to compare options._persist.version to persistModComponentOptionsConfig.version as it would make this selector much more change-tolerant
There was a problem hiding this comment.
That makes sense - I had copied this from the modComponentSelectors code
There was a problem hiding this comment.
I think that might be a problem for test ergonomics given that test stores typically don't have the persistence props attached to the slices? https://github.com/pixiebrix/pixiebrix-extension/blob/feature%2Fdevex-mod-instance/src/pageEditor/testHelpers.ts#L38-L38
Let's punt to be separate work?
| export const selectModInstanceMap = createSelector( | ||
| selectModInstances, | ||
| (modInstances) => | ||
| new Map( | ||
| modInstances.map((modInstance) => [ | ||
| modInstance.definition.metadata.id, | ||
| modInstance, | ||
| ]), | ||
| ), | ||
| ); |
There was a problem hiding this comment.
I would avoid returning a Map from a redux selector. We shouldn't ever store a Map in redux, so we would still be adapting redux state even after a redux migration
There was a problem hiding this comment.
I would avoid returning a Map from a redux selector. We shouldn't ever store a Map in redux, so we would still be adapting redux state even after a redux migration
Could you clarify your concern for returning a Map? The output of selectors don't need to be serializable
We shouldn't ever store a Map in redux, so we would still be adapting redux state even after a redux migration
My understanding of selectors is that they adapt the persisted redux state into a form that's easy for the application to use. They allow you, for example, to have the data denormalized in the redux state (to avoid invalid data) and then normalize it for the application to use
Not sure what you'd recommend instead. The options off the top of my head are:
- Creating a selector factory that takes an id and produces a selector?
- Returning an array having the callsite do an O(n) search?
- Having this return an object for the callsite to do the lookup? From an ergonomics perspective, a Map is preferable to using an object as a map
There was a problem hiding this comment.
If you intend the data to never be stored in Redux, then I can withdraw my objection.
My understanding was the intention of these selectors in particular was to serve as a stand-in for the eventual redux migration. Since we can't store Maps, we would have to rewrite both the selector and the consumers. Not an issue if the selector will always contain the logic of creating the Map
There was a problem hiding this comment.
The redux state will be an array of ModInstances which are serializable. And there can be a selector for that.
But business logic that uses the slice has a need to be able to get the ModInstance for a given mod id.
So it's useful to also have a selector that returns a lookup. And it's useful to memoize that since multiple components on the screen may be performing the lookup. Our code on main has to perform a lot of O(n) lookups of mod components
There's also a hook if the users needs a specific mod id (or undefined) vs. needing the map. Some of the activation methods (e.g., useActivateMod) and mods screen method need the Map because the returned callbacks take mod id as an argument. So we can't use the hook in those cases
There was a problem hiding this comment.
Agreed. I'm fine with the Map in that case. Just wanted to call attention in case the plan was to store the Map
There was a problem hiding this comment.
As you mention, when we go to migrate the redux state, there will be a question of storing as an array vs. object keyed by mod registry id. My general feeling is array works best since the lookups would generally be by:
- all mod instances
- registry id
- deployment id
But keep as array leaves open if we ever want to support multiple mod instances for a given mod id. And for these lookups we can just use reselect
What does this PR do?
ModInstancetypeModInstanceto simplify some React code:useActivateModuseReactivateModuseActivateModWizardDiscussion
pixiebrix-extension/src/contentScript/lifecycle.ts
Line 442 in 955553c
Future Work
Prioritizing changes that simplify deployment code, doesn't require a migration, and avoids the Page Editor:
ModInstance[]instead ofActivatedModComponent[]: Slice 2: UseModInstancetype in mod deployment code (2/3) #9190ModInstance[]instead of passing aroundActivatedModComponent[]. E.g., see buildModsList, buildGetModActivationStatus, etc.: Slice 3: UseModInstancetype on the mods screen (3/3) #9191Consider adding a wrapper that wraps
ModInstancewith helper methods for derived data/properties. E.g.,modId,isPaused, etc. without duplicating the dataFor more information on our expectations for the PR process, see the
code review principles doc