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

Clean up dependencies for @deephaven/redux #730

Open
mofojed opened this issue Aug 25, 2022 · 3 comments
Open

Clean up dependencies for @deephaven/redux #730

mofojed opened this issue Aug 25, 2022 · 3 comments
Labels
enhancement New feature or request
Milestone

Comments

@mofojed
Copy link
Member

mofojed commented Aug 25, 2022

@deephaven/redux should not have any dependencies on other @deephaven packages. Essentially it just needs the middleware stuff and registry, and the rest should be actions/selector exported by other packages.

E.g. The setCommandHistoryStorage action should be registered by the @deephaven/dashboard-core-plugins (specifically would be @deephaven/console-plugin if we had that).

@mofojed mofojed added enhancement New feature or request triage Issue requires triage labels Aug 25, 2022
@mofojed mofojed mentioned this issue Aug 25, 2022
@mofojed mofojed self-assigned this Aug 31, 2022
@mofojed mofojed removed the triage Issue requires triage label Aug 31, 2022
mofojed added a commit to mofojed/web-client-ui that referenced this issue Sep 1, 2022
- Partial deephaven#730
- Remove references to @deephaven/redux from @deephaven/chart and @deephaven/iris-grid
- Create a `Settings` in jsapi-utils that captures all the formatting settings, use that instead of WorkspaceSettings
- Still need to clean up WorkspaceSettings, and the actions registered (e.g. setCommandHistoryStorage should be exported from dashboard-core-plugins, not from redux), but will do that later as this is lower risk
@mattrunyon
Copy link
Collaborator

mattrunyon commented Sep 1, 2022

I think it's fine for redux to reference types from other deephaven packages, but we don't want it the other way around where anything that isn't the main app (or dashboard plugins maybe) referencing redux. Really it depends on how we want to separate it

  1. Redux references packages, packages don't reference redux

I think this would be fine because the redux store is an app specific implementation. You can pull in chart without redux and it should work just fine. Any redux connect/middleware/updates should be handled at the app level (or dashboard core plugins) which then just delegates the updated state to the component which does not know about redux

  1. Packages reference redux, redux doesn't reference packages

This would couple our packages to redux which I think is the wrong move.

  1. Neither references each other

This could also be fine and might be the safest overall. Involves a bit of duplication of types, but also prevents accidentally changing types in a package and not realizing it modified the redux store or workspace storage.

Since we have migrations for workspace data, option 3 is probably what we want? Then if something in chart gets modified, TS should warn us if redux expects a different type instead of relying on the type from chart

mofojed added a commit that referenced this issue Sep 2, 2022
- Partial #730
- Remove references to @deephaven/redux from @deephaven/chart and @deephaven/iris-grid
- Create a `Settings` in jsapi-utils that captures all the formatting settings, use that instead of WorkspaceSettings
- Still need to clean up WorkspaceSettings, and the actions registered (e.g. setCommandHistoryStorage should be exported from dashboard-core-plugins, not from redux), but will do that later as this is lower risk
- In a future change, we should move actions from `@deephaven/redux` to dashboard-core-plugins (such as `setCommandHistoryStorage`).
@mofojed mofojed added this to the Backlog milestone Oct 7, 2022
@mofojed mofojed removed their assignment Oct 7, 2022
@mofojed
Copy link
Member Author

mofojed commented Jul 6, 2023

It's come to light that @deephaven/redux is more of an app level package, so it should be fine if it depends on app packages. Types shouldn't necessarily be defined in @deephaven/redux. Other deephaven packages should not depend on redux.

@mattrunyon
Copy link
Collaborator

One thought I've had towards cleaning up redux in general is making the @deephaven/redux package just utils (and maybe naming @deephaven/redux-utils or base or something) and not any selectors/actions/reducers.

Then each package can have its own selectors/actions/reducers which use the @deephaven/redux-utils package to register them (like we do now). In each package (like dashboard as an example), we would essentially create what redux toolkit calls a "slice". Anything that needs to work with that slice would need to import the selectors/actions from the specific package anyway.

Then inside code-studio we have the store setup and any other app specific redux. Any redux that is shared between DHC and DHE would be in a non-app level package. Then we still have a central-ish definition of the store (in the app package) without the current difficulty of defining the shape of the whole store without proper types in a lot of places (like workspace and dashboard data are mostly unknowns we cast when selecting)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants