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

@connect + contextType breaks with React 16.6 #1067

Closed
Philipp91 opened this issue Oct 28, 2018 · 12 comments
Closed

@connect + contextType breaks with React 16.6 #1067

Philipp91 opened this issue Oct 28, 2018 · 12 comments

Comments

@Philipp91
Copy link

Philipp91 commented Oct 28, 2018

(I can provide a reproducible example if there's a simple test project that I can base it on.)

Essentially, I'm using React 16.6 and doing this:

const MyContext = React.createContext();

@connect(...)
class MyComponent extends React.Component {
    static contextType = MyContext;
    render() {
        return <div>Hello {this.context}</div>;
    }
}

ReactDOM.render(<MyContext.Provider value="world"><MyComponent/></MyContext.Provider/>, ...);

And it fails with an error. In my concrete example, the value isn't "world" of course, but it happens to be undefined, so I'm getting a TypeError: cannot read property 'store' of undefined here because context===undefined. I guess if I were to pass "world" as the context, it would instead fail the invariant just below.

The problem is that the <Connect(MyComponent)> component gets its context populated based on my static contextType, even though it's setting its own legacy context for the usual redux plumbing. But the new context takes precedence. This happens because <Connect(MyComponent)> declares the legacy contextTypes and additionally hoists the contextType from the nested component. This is ultimately a bug in the hoist-non-react-statics library and I have already sent a fix there: mridgway/hoist-non-react-statics#62

Please let me know if you disagree with the fix. In case they don't merge it soon, react-redux could work around the issue by doing something like this (I can send a PR):

const hoisted = hoistStatics(Connect, WrappedComponent);
delete hoisted.contextType;
return hoisted;
@markerikson
Copy link
Contributor

This is likely due to one of the known bugs in React where mixing legacy context and createContext cause things to work wrong. Also, while it may not be directly related, we don't recommend or support using connect as a decorator.

If there's an easy fix, we can potentially look at it, but otherwise my advice is to avoid using contextType on a component until React-Redux v6 is out, and you've upgraded to that.

@DJTB
Copy link

DJTB commented Oct 30, 2018

Can confirm using the decorator is not related here.
Using connect() around a class with static contextType = myContext is resulting in the same undefined store error.

@CrOrc
Copy link

CrOrc commented Oct 30, 2018

Until react-redux v6 is released the easiest workaround is to wrap component with contextType into function component

const MyContext = React.createContext();

class MyComponent extends React.Component {
    static contextType = MyContext;
    render() {
        return <div>Hello {this.context}</div>;
    }
}
const MyConnectedComponent = connect(...)(props=><MyComponent {...props/>)

@Philipp91
Copy link
Author

Yeah, the connect() remark was just an fyi upon reading my code, right? I tried to find more information on this, but couldn't find any (official or unofficial) recommendation about this with a quick web search.

The fix has been merged upstream and is released in hoist-non-react-statics:3.1.0. So react-redux could be fixed by bumping the package-lock.json here and releasing a new version. I'm not sure if NPM allows users to override the hoist-non-react-statics version in their own (outer) package.json somehow.

@timdorr
Copy link
Member

timdorr commented Oct 30, 2018

The package-lock.json files aren't provided in npm packages, nor are they used when installing. So, as long as another dependency isn't blocking the upgrade, you can use that version with react-redux.

@timdorr timdorr closed this as completed Oct 30, 2018
@markerikson markerikson reopened this Nov 10, 2018
@markerikson
Copy link
Contributor

markerikson commented Nov 10, 2018

Looks like hoist-non-react-statics@3.1.0 ought to fix this: mridgway/hoist-non-react-statics#62 .

Tim, can we get that into a 5.1.1 along with the isPlainObject fix?

edit

derp, I should read things more carefully. Someone said 3.1.0 already

@timdorr , we have a direct dep on h-s-n-r@^3.0.1 currently. Can we at least bump that with the next build we release?

@timdorr timdorr closed this as completed Nov 10, 2018
@timdorr
Copy link
Member

timdorr commented Nov 10, 2018

Adding this to 5.1.1

@empav
Copy link

empav commented Dec 20, 2018

Any news? Using redux and connect a parent component to it, the static contextType does not work for me either:
react 16.7.0
react-dom: 16.7.0
react-redux: 6.0.0

@markerikson
Copy link
Contributor

markerikson commented Dec 20, 2018

@epavan : Please provide a project that specifically demonstrates this problem.

@empav
Copy link

empav commented Dec 22, 2018

screenshot 2018-12-22 at 12 58 20

Can't access this.context.object at all! Tried also the nonstatic version.

/* eslint no-unused-vars:"off" */
import React, { Component } from 'react';
import ReactDOM from 'react-dom';

const SomeContext = React.createContext();

class Provider extends React.Component {
  state = {
    object: { say: 'Hi there' }
  };

  render() {
    return <SomeContext.Provider value={this.state}>{this.props.children}</SomeContext.Provider>;
  }
}

class Workareas extends React.Component {
  static contextType = SomeContext;

  render() {
    return (
      <Provider>
        <pre>Context value: {JSON.stringify(this.context)}</pre>
      </Provider>
    );
  }
}

export default Workareas;

@markerikson
Copy link
Contributor

@epavan : can you please turn that into a CodeSandbox project that's runnable?

@empav
Copy link

empav commented Dec 22, 2018

I'm doubtful with some incompatibility with react-router 3 which uses context to provide router/route params. I'll try to codesandbox it.

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

No branches or pull requests

6 participants