Skip to content

fix: Move @deephaven/redux to be a dependency instead#1249

Merged
mofojed merged 1 commit intodeephaven:mainfrom
mofojed:fix-peer-deps
Apr 25, 2023
Merged

fix: Move @deephaven/redux to be a dependency instead#1249
mofojed merged 1 commit intodeephaven:mainfrom
mofojed:fix-peer-deps

Conversation

@mofojed
Copy link
Copy Markdown
Member

@mofojed mofojed commented Apr 25, 2023

  • Issue when trying to do an npm install in another project that depended on @deephaven/dashboard, as the peerDependency would not resolve the "file:../redux" path
    • Not sure if lerna or npm should automatically be replacing those at publish time?
  • Was adding redux as a peerDep because we only want one instance of the redux store; otherwise weird bugs can occur that are difficult to track down
  • It was already the case with @deephaven/dashboard-core-plugins that it was just a dependency, not a peerDependency. Enterprise handled it just by doing deduplication
  • Just move @deephaven/redux to be a dependency instead of a peerDependency

- Issue when trying to do an `npm install` in another project that depended on @deephaven/dashboard, as the peerDependency would not resolve the "file:../redux" path
  - Not sure if lerna or npm should automatically be replacing those at publish time?
- Was adding redux as a peerDep because we only want one instance of the redux store; otherwise weird bugs can occur that are difficult to track down
- It was already the case with @deephaven/dashboard-core-plugins that it was just a dependency, not a peerDependency. Enterprise handled it just by doing deduplication
- Just move @deephaven/redux to be a dependency instead of a peerDependency
@mofojed mofojed added the bug Something isn't working label Apr 25, 2023
@mofojed mofojed requested review from bmingles and vbabich April 25, 2023 20:34
@mofojed mofojed self-assigned this Apr 25, 2023
Copy link
Copy Markdown
Contributor

@bmingles bmingles left a comment

Choose a reason for hiding this comment

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

Looks good to me

@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 25, 2023

Codecov Report

Merging #1249 (aa02c0b) into main (de7a110) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #1249   +/-   ##
=======================================
  Coverage   45.67%   45.67%           
=======================================
  Files         473      473           
  Lines       33703    33703           
  Branches     8455     8455           
=======================================
  Hits        15394    15394           
  Misses      18258    18258           
  Partials       51       51           
Flag Coverage Δ
unit 45.67% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@mofojed
Copy link
Copy Markdown
Member Author

mofojed commented Apr 25, 2023

Published an alpha package, 0.37.1-redux-peer-dep.0. Verified that I was able to complete an npm install successfully on Enterprise using that alpha version of the packages, e.g.:

  "dependencies": {
    "@deephaven/chart": "0.37.1-redux-peer-dep.0",
    "@deephaven/components": "0.37.1-redux-peer-dep.0",
    "@deephaven/console": "0.37.1-redux-peer-dep.0",
    "@deephaven/dashboard": "0.37.1-redux-peer-dep.0",
    ....

@mofojed mofojed merged commit 3f24e11 into deephaven:main Apr 25, 2023
@mofojed mofojed deleted the fix-peer-deps branch April 25, 2023 20:51
@github-actions github-actions Bot locked and limited conversation to collaborators Apr 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants