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

fix: update react-redux to v7 #78

Closed
wants to merge 3 commits into from

Conversation

vivek1729
Copy link
Collaborator

@vivek1729 vivek1729 commented Aug 26, 2021

To think through some of the challenges in upgrading react-redux to v7, I decided to scaffold a PR and see how far I get in this effort. Detailed info on the release notes and breaking changes for v7 can be found here. Since, the only public breaking change is the min peer dependency of react package from 16.4 to 16.8.4, I didn't bump react* packages in this repo since they seem to be at a compatible version already (16.13.1).

The decision to go with v7.2.0 was deliberate because there had been a few minor releases and this contains some important bug fixes and memory leak mitigations.

Learnings

Within the ouptuts repo, the jupyter-widgets package depends upon react-redux support. So, I imagine that all the other packages maintained in this repo should be unaffected by the upgrade.

To validate the package, I decided to update our host app to react-redux v7.2.0 as well and I seem to be hitting this issue when attempting to render ipywidgets:

image

We lazy load the WidgetDisplay component from the jupyter widgets package and from the error it seems like in the TransformMedia component, it loses context of the store. Here's how we use the TransformMedia component:

<React.Fragment>
   <div className={"allow-pointer-events"}>
      {directOutputs.map((output, index) => (
            <Output output={output} key={index}>
              <TransformMedia output_type={"display_data"} id={id} contentRef={contentRef} />
              <TransformMedia output_type={"execute_result"} id={id} contentRef={contentRef} />
                 ...
            </Output>
          ))}...

Based on some troubleshooting guidance (here and here), I was able to explore the idea of providing the store directly as a prop and make some progress. However, I don't know if that is required since our App already has a top level Provider component that should make the store available to all children elements.

I think it might be important to consider the inter dependency and use cases of some packages in the nteract/nteract monorepo like the TransformMedia component when upgrading the outputs repo.

@vivek1729
Copy link
Collaborator Author

@captainsafia , I was able to resolve the issue (about missing store) in our code base. It turns out this was popping up because of having multiple copies of the react-redux package. Adding this package as a peer dependency in the widgets project alleviated this problem. This ensures that the consumers leverage their single instance of react-redux. We should follow this pattern for other connected components as well.

As I side note, I found the npm pack command quite useful in testing out the whole npm publish/install flow locally for the widgets package (source)

@captainsafia captainsafia changed the title DRAFT (Do not merge) Attempt redux7 update fix: update react-redux to v7 Sep 2, 2021
@captainsafia
Copy link
Member

Closing in favor of #82 since I had to rebase and update test snapshots and do not have access to the original branch.

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.

None yet

2 participants