nteract / outputs

A collection of React components for displaying rich Jupyter display objects
BSD 3-Clause "New" or "Revised" License
26 stars 19 forks source link

fix: update react-redux to v7 #78

Closed vivek1729 closed 3 years ago

vivek1729 commented 3 years ago

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 commented 3 years ago

@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 commented 3 years ago

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