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

Add Studio V2 store with common, console and plugin reducers #1280

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

GnsP
Copy link
Collaborator

@GnsP GnsP commented Jan 17, 2025

Add Studio V2 store with common, console and plugin reducers

Description

As a part of migrating Studio to react, we start by moving the state stores to typescript and redux.

PR Type

  • Bug Fix
  • Feature
  • Build Fix
  • Testing
  • General Improvement
  • Cherry Pick

Links

Jira: Jira issue #

Test Plan

Existing tests should pass.

Screenshots

NA

Sorry, something went wrong.

@GnsP GnsP added the build triggers github action label Jan 21, 2025
@GnsP GnsP force-pushed the studio-plugins-store branch 2 times, most recently from a0fa859 to 50c706f Compare January 21, 2025 08:32
@@ -0,0 +1,27 @@
/*
* Copyright © 2024 Cask Data, Inc.
Copy link
Member

Choose a reason for hiding this comment

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

nit: 2025 in all new files

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, will update the new files. Thanks for pointing out !

*
* @returns The cloned copy of the object.
*/
export function cloneAndApplyMutation<T>(object: T, mutation: (draft: T) => any): T {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is fine, but it could trigger re-renders on parts that haven't changed. Another approach would be to define the mutation (or call them helper) functions to return Partial<T>. Then you could use the spread operator to create the new state instance like normal.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@njbriggs
Deep cloning the state may trigger re-renders. However returning a Partial<T> and using spread operator would not work for mutations with delete operator.

(Also, the Partial<T> in typescript performs a shallow partial conversion.)

I was considering the idea of using Proxy to intercept mutating operations (both set and delete) and capture them in a diff object, and then apply the diff on the actual state.
We can use the immer library (specifically, the immer.produce function) to do this; tradeoff -- it's 3Kb in size. Alternatively, we can implement a simpler version of immer.produce API, supporting only property mutations on plain objects and arrays.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We already have immer in yarn.lock. We also have immutable. Would we be better off using one of those or even a Map for this piece of state?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Then I think it would be better to use immer. It has a drop-in replacement for this function. Using immutable would require more changes.

@GnsP GnsP force-pushed the studio-plugins-store branch from 50c706f to 2cccffc Compare January 28, 2025 04:00
@GnsP GnsP requested a review from radhikav1 January 28, 2025 05:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build triggers github action
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants