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

Update binding modelling in client state #986

Merged
merged 69 commits into from
Mar 20, 2024

Conversation

kiahna-tucker
Copy link
Member

@kiahna-tucker kiahna-tucker commented Feb 16, 2024

Issues

The issues directly below are completely resolved by this PR:
#935

Changes

935

The following features are included in this PR:

  • Enable the editing of multiple bindings that map to the same collection.

  • Create a binding store that houses binding-related state.

    • Notable store state:

      • backfilledBindings -- the analog of resource config store state property, backfilledBindings, which is an array of binding UUIDs.

      • bindings -- the analog of resource config store state property, collection, which is dictionary keyed by the name of the collection associated with a binding with the array of related binding UUIDs as a value.

      • currentBinding -- the analog of resource config store state property, currentCollection, which is an object containing a reference to the binding UUID and name of the collection associated with the selected binding.

      • resourceConfigs -- the analog of resource config store state property, resourceConfig, which is dictionary keyed by binding UUID with a modified ResourceConfig object.

    • Notable store actions:

      • addEmptyBindings -- the analog of resource config store action, preFillEmptyCollections.

      • evaluateDiscoveredBindings -- the analog of resource config store action, evaluateDiscoveredBindings, which encapsulates the functionality of setDiscoveredCollections and resetConfigAndCollections.

      • prefillBindingDependentState -- the combination of resource config store actions, prefillResourceConfig and prefillBackfilledCollections, with expanded scope that initializes the time travel-related state in materialization workflows. Formerly, the latter was done in useInitializeTaskDraft.

      • prefillResourceConfigs -- the partition of the second half of resource config store action, setResourceConfig, which updated the resource config dictionary when a capture is linked to a materialization (i.e., src/components/materialization/SourceCapture/AddSourceCaptureToSpecButton.tsx) and bindings are added to the specification via the collection selector (i.e., src/components/editor/Bindings/UpdateResourceConfigButton.tsx).

      • removeDiscoveredBindings -- the analog of resource config store action, resetResourceConfigAndCollections.

      • updateResourceConfig -- the combination of resource config store actions, updateResourceConfig and the partition of the first half of setResourceConfig, which updated the resource config dictionary when editing a binding's resource config form (i.e., src/components/collection/ResourceConfigForm.tsx).

  • Remove the resource configuration store.

  • Move field selection and time travel-related state from the bindings editor store into the new bindings store.

  • Update the logic of getBindingIndex, generateTaskSpec, useServerUpdateRequiredMonitor to account for the existence of multiple bindings that map to the same collection.

  • Use the binding UUID as the path of the Monaco editor used to display/edit the collection specification in the right-hand pane of the binding selector.

  • Preserve the value of the active hydration-related, binding state property by default when the state is reset. An optional parameter, resetActive, was added to the definition of the resetState binding store action to enable resetting the aforementioned property when necessary. Presently, active should only be reset when a given workflow is exited.

Tests

Manually tested

A myriad of binding-related scenarios in the create and workflows were tested; listing them entirely would be tedious and futile. Consequently, this section will document any scenarios that require retesting after the following commit hash: fbaaaa6151bbf3ffaf0c0f8e44b9316429e8d6fa.

Automated tests

  • Updated the binding editor store initialization snapshot after removing field selection and time travel state slices.

  • Lightly expanded the test coverage of the getBindingIndex and generateTaskSpec workflow utils.

Screenshots

N/A

@travjenkins

This comment was marked as resolved.

Comment on lines +184 to +188
? getBindingIndex(
draftSpec.bindings,
collectionName,
iteratedIndex
)
Copy link
Member

Choose a reason for hiding this comment

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

This is okay for now. But this approach needs to change somewhat quickly.

We should not be calling this inside of a loop. Instead we should handle fetching what collections are at what index before the loop. That way we are not running through a mapping and filtering of all the bindings every time for every binding.

Copy link
Member Author

@kiahna-tucker kiahna-tucker Mar 14, 2024

Choose a reason for hiding this comment

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

Sure. This is worth a discussion around expectations; primarily, whether getBindingIndex should be used in this function at all. I am inclined to say it should not be used here.

As aforementioned, I aspired to make as minimal changes as possible with the binding model refactoring. The binding index was evaluated in a similar fashion previously, so I lightly modified it in this PR.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah - we really should strive to not use it here at all. Can you throw in a TODO here and explain what the future work could be.

Comment on lines +44 to +50
const selectedValue = useMemo(
() =>
Object.hasOwn(selections, bindingUUID)
? selections[bindingUUID][field]
: null,
[bindingUUID, field, selections]
);
Copy link
Member

Choose a reason for hiding this comment

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

Eventually we should pass bindingUUID into the store's hook when fetching bindingUUID since we never reference it any other way.

travjenkins

This comment was marked as resolved.

@travjenkins
Copy link
Member

Ran a quick profile - these would be the places to start looking

image

Copy link
Member

@travjenkins travjenkins left a comment

Choose a reason for hiding this comment

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

A couple questions. Also - there is a quality check failure.

Ran local tests and everything looks good. Performance is back to being totally reasonable.

Copy link
Member

@travjenkins travjenkins left a comment

Choose a reason for hiding this comment

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

lgtm - please fix things we discussed in person. Marking as approved so you can merge whenever you want.

@kiahna-tucker kiahna-tucker merged commit d602d16 into main Mar 20, 2024
3 checks passed
@kiahna-tucker kiahna-tucker deleted the kiahna-tucker/refactor/bindings-data-model branch March 20, 2024 15:30
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.

2 participants