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

Is there a way to access new context api within ComponentDidMount? #12397

Closed
grillorafael opened this Issue Mar 18, 2018 · 28 comments

Comments

Projects
None yet
@grillorafael
Copy link

grillorafael commented Mar 18, 2018

We are building a react mapbox gl module and we use clone and inject props today.

We were looking into using the 16.2.0 context api but I saw that it will have a new one on 16.3.0 but I can’t seem to find a way to read context details
On componentDidMount lifecycle (which makes sense for me to use on the map implementation).

Is there a way around this ?

@grillorafael

This comment has been minimized.

Copy link

grillorafael commented Mar 18, 2018

Adding a example that I'm trying to have this working: https://codesandbox.io/s/l20yn296w7

EDIT: Following the guidelines from https://github.com/reactjs/rfcs/blob/master/text/0002-new-version-of-context.md#class-based-api

@TrySound

This comment has been minimized.

Copy link
Contributor

TrySound commented Mar 18, 2018

This is how it can be achieved with new api.

class BaseMapElement extends React.Component {
  componentDidMount() {
    console.log(this.props.context);
  }

  render() {
    return null;
  }
}

const MapElement = () => (
  <Context.Consumer>
    {context =>
      <BaseMapElement context={context} />
    }
  </Context.Consumer>
)
@grillorafael

This comment has been minimized.

Copy link

grillorafael commented Mar 18, 2018

So the only way to access through componentDidMount is to redirect into Props?

Edit: Changed to componentDidMount

@jquense

This comment has been minimized.

Copy link
Collaborator

jquense commented Mar 18, 2018

The higher order component that redirecs to props is a good pattern,but for cases where don't want the extra component usually wat I do is store the context value on the component instance like: this.contextValue = value then access it in the life cycle hooks. It's a bit ugly but that's ok generally I think since your opting out of the nicer, hoc, pattern as an optimization

@porfirioribeiro

This comment has been minimized.

Copy link

porfirioribeiro commented Mar 22, 2018

I'm facing the same dilemma.
There should be a easy way to access or call context in life-cycle methods.
We might need to initialize stuff, check and fetch data or even clean-up on unmount.
People are doing it now with the current supposedly broken context.
Yes it can be solved by wrapping our component in another component! But feels like a workaround more than a solution.

@gaearon

This comment has been minimized.

Copy link
Member

gaearon commented Mar 22, 2018

store the context value on the component instance like: this.contextValue = value then access it in the life cycle hooks

I’m pretty sure this is unsafe in async mode. Please don’t do this. cc @acdlite

@grillorafael

This comment has been minimized.

Copy link

grillorafael commented Mar 22, 2018

Yes it can be solved by wrapping our component in another component! But feels like a workaround more than a solution.

I agree with that. Would be nice to natively access through componentDidMount and componentWillUnmount to be able to initialize/cleanup things,

@acdlite

This comment has been minimized.

Copy link
Member

acdlite commented Mar 22, 2018

In general, using instance vars to be clever and cheat the normal data flow is going to cause problems in async. Just don’t. It’s a bit confusing today, because instance vars are the only way to do certain things, like timers. Before we release async we’ll publish clearer recommendations — and one day we’ll have a new component API that doesn’t rely so much on instances.

tl;dr: Use a props indirection. And don’t worry too much about the extra component.

@jquense

This comment has been minimized.

Copy link
Collaborator

jquense commented Mar 22, 2018

I’m pretty sure this is unsafe in async mode. Please don’t do this.

How would this be unsafe (and in what way)? It's faily unclear in all this talk about async mode, what "unsafe" means. It's starting to feel like a boogyman who behavior is irrational and unpredictable, which doesn't fill folks with a lot of assurance in a system that is generally liked for it's straightforward, easily understandable, data flow model. I feels like components are back in pre 0.13 land where they are magic objects again.

It's also easy to say "Just add another component", but that's often onerous, and introduces it's own category of bugs and challenges. I't starts to feel like folks have to invent abstractions over a react API in order to be "safe".

@jquense

This comment has been minimized.

Copy link
Collaborator

jquense commented Mar 22, 2018

sorry ^ if the above sounds annoyed/angry, i didn't intend that tone! on a phhhhooone

@grillorafael

This comment has been minimized.

Copy link

grillorafael commented Mar 22, 2018

It did sound angry haha but I get your point and I share your questions about how/why it will be unsafe

@gaearon

This comment has been minimized.

Copy link
Member

gaearon commented Mar 23, 2018

It's starting to feel like a boogyman who behavior is irrational and unpredictable

I’m sorry, but we’ve been working on a guidance with specific suggestions for over a month now. Please give us time to collect and publish them as a blog post. It is also difficult to discuss without actual alphas to play with, which is also something we’ve been working hard on.

So we have to either not say anything at all, or warn in advance about things that won’t work well. We err on the side of warning but I can see how it can look like we’re making it more difficult for you. I’m sure that once the code is out and you can play with it, you’ll see what we mean, and it’ll make more sense.

How would this be unsafe (and in what way)?

Did you get a chance to watch my talk? This will be a bit hard to explain if you haven’t seen the second part of it, because it wouldn’t be clear why we are doing this. So I’m asking you to watch it. I hope that my talk will convince you we’re aiming to solve a broad class of problems that plagued React since the beginning, and that these features are worth revisiting some assumptions we might have gotten used to.

Assuming you’ve seen the talk, here’s a more specific explanation about this particular case. In order to be able to “suspend” rendering like I showed in the demo, React needs to be able to call render() at any point in time, potentially with different props. For example if may set this.props and this.state to props from a new screen (that’s being loaded), call render, but then set it to old this.props and this.state when rendering in response to an interaction on the current version of the tree (e.g. if I press on something while the new screen is loading).

In async, the rule of thumb is: only lifecycles like componentDidMount, componentDidUpdate, and componentWillUnmount and ref callbacks execute at well-defined points in time with props and state that correspond to what’s on the screen. Luckily we only have a few other lifecycles that don’t neatly fit into this picture, and we are introducing better alternatives for them (getDerivedPropsFromState, getSnapshotBeforeUpdate). This will be a gradual migration. Again, it’s all going to be in the blog post.

Now to the root of this issue. In async mode, React doesn’t make guarantees about when and in which order it calls the render method. Which is really something React never guaranteed in the first place—it just happened to be the same order every time. Storing a field in render and then reading it in a lifecycle is not “safe” because you might store a field at the time React called render with different props (such as for a suspended tree that isn’t ready yet).

@gaearon

This comment has been minimized.

Copy link
Member

gaearon commented Mar 23, 2018

There should be a easy way to access or call context in life-cycle methods.
Yes it can be solved by wrapping our component in another component! But feels like a workaround more than a solution.

I understand it feels like it’s an extra wrapper, but it’s what makes the new context fast. If we didn’t have explicit wrapper nodes in the tree we wouldn’t be able to quickly “find” components that need to update.

If you need to access context in lifecycle, take it as a prop.

class Button extends React.Component {
  componentDidMount() {
    alert(this.props.theme);
  }

  render() {
    const { theme, children } = this.props;
    return (
      <button className={theme ? 'dark' : 'light'}>
        {children}
      </button>
    );
  }
}

export default React.forwardRef((props, ref) => (
  <ThemeContext.Consumer>
    {theme => <Button {...props} theme={theme} ref={ref} />}
  </ThemeContext.Consumer>
));

This is almost the same amount of lines as a contextTypes definition.

@bvaughn

This comment has been minimized.

Copy link
Contributor

bvaughn commented Mar 23, 2018

Yes it can be solved by wrapping our component in another component! But feels like a workaround more than a solution.

Just wanted to second what Dan said that the child function / render prop approach is the official API for the new context- so please use it and let React worry about making sure it's fast. (It will be!)

How would this be unsafe (and in what way)?

The draft Strict Mode docs also touch on some of why mutating the instance (which is just another type of side effect) is dangerous in async mode.

@grillorafael

This comment has been minimized.

Copy link

grillorafael commented Mar 24, 2018

We have a experimental branch following the guidelines proposed here. Can anyone have a look to see if it make sense? commodityvectors/react-mapbox-gl#11

@bvaughn

This comment has been minimized.

Copy link
Contributor

bvaughn commented Mar 24, 2018

I'm not familiar with this library, so I don't know if people ever make use of refs with its components- but if they did, the withContext mixin on that PR might be a good use case for the new forwardRef API.

@grillorafael

This comment has been minimized.

Copy link

grillorafael commented Mar 25, 2018

That make sense. Thanks for the reference. I'm going to close this issue for now.

@lostpebble

This comment has been minimized.

Copy link

lostpebble commented Apr 25, 2018

Just ran into this issue because I've been trying to see if I can achieve the same thing.

So, from what I can gather from all this, it is not possible in the component which makes use of the Context.Consumer to access the API outside of the child render function. I've come up with something which may work to make this all a little easier though (would really appreciate some feedback if this is not good practice for whatever reason):

const MapElement = (props) => (
  <Context.Consumer>
    {context =>
      <RunOnLifecycle
        runOnMount={() => { /*use context*/ }}
        runOnUnMount={() => { /*use context*/ }}
        runOnUpdate={(prevProps) => { /*use context - compare prevProps with props */ }}
        { ...props }
      />
    }
  </Context.Consumer>
)

And that helper component <RunOnLifecycle/> :

export interface IPropsRunOnLifecycle {
  runOnMount?: () => void;
  runOnUpdate?: (prevProps: object) => void;
  runOnUnMount?: () => void;
  children?: JSX.Element | ReactNode;
  [prop: string]: any;
}

export class RunOnLifecycle extends React.Component<IPropsRunOnLifecycle> {
  componentDidUpdate(prevProps, prevState, snapshot) {
    if (this.props.runOnUpdate != null) {
      this.props.runOnUpdate(prevProps);
    }
  }

  componentDidMount() {
    if (this.props.runOnMount != null) {
      this.props.runOnMount();
    }
  }

  componentWillUnmount() {
    if (this.props.runOnUnMount != null) {
      this.props.runOnUnMount();
    }
  }

  render() { return this.props.children || null; }
}

Wondering if this is going to cause any headaches down the line. Still feels like pretty standard React, if somewhat of a hack.

@bvaughn

This comment has been minimized.

Copy link
Contributor

bvaughn commented Apr 25, 2018

There are some subtle differences that might make that approach a bad idea. For example, if MapElement was a class component that used refs, the refs would not yet be set when the runOnMount callback was run.

😄 I would suggest using a HOC approach for this instead:
https://reactjs.org/docs/context.html#consuming-context-with-a-hoc

The only real downside to using a HOC for this sort of thing has been mitigated by the forwardRef API:
https://reactjs.org/docs/react-api.html#reactforwardref

@grillorafael

This comment has been minimized.

Copy link

grillorafael commented Apr 25, 2018

We took the approach like the react docs and what people said here. It is working well for us so far.

https://github.com/commodityvectors/react-mapbox-gl/blob/master/src/Map.js#L63

@lostpebble

This comment has been minimized.

Copy link

lostpebble commented Apr 25, 2018

There are some subtle differences that might make that approach a bad idea. For example, if MapElement was a class component that used refs, the refs would not yet be set when the runOnMount callback was run.

Thanks for the feedback @bvaughn . At the moment I'm using it purely as a kind of state proxy component which adds / removes things from the UI depending on what is mounted within the context tree. Kind of like portals but within the React component tree. So not actually rendering children or dealing with refs at all.

Will keep in mind if I need to do anything that interacts with refs.

@AmnArora

This comment has been minimized.

Copy link

AmnArora commented Jul 3, 2018

Hey everyone,

I need context data in lifecycle methods. so, I followed the HOC approach after seeing the first few comments and passed the context data as props.
Everything is working as expected. But now, I want to write unit test cases for the components but I am unable to do so.

I would really appreciate it if someone could share how I can write test cases for this scenario.

I am using enzyme, enzyme-adapter-react-16 and jest but having some troubles in doing so.

@pgarciacamou

This comment has been minimized.

Copy link

pgarciacamou commented Jul 3, 2018

@AmnArora

At the company I work for, we do the following (note, this might not be the consensus), we export the "naked" component as well and then import it in our tests and manually pass the props.

E.g.

// MyComponent.js
export class MyComponent extends Component { /* ... */ }
export default HOC()(MyComponent)

// MyComponent.spec.js
import { MyComponent } from '...'

// OtherComponents.js
import MyComponent from '...'

Also, adding to this discussion, we encountered the same issue and created this https://www.npmjs.com/package/react-context-consumer-hoc that consumes multiple context.

@bvaughn

This comment has been minimized.

Copy link
Contributor

bvaughn commented Jul 3, 2018

Everything is working as expected. But now, I want to write unit test cases for the components but I am unable to do so.

@AmnArora Why are you unable to write a unit test? What have you tried? What error are you seeing?

@AmnArora

This comment has been minimized.

Copy link

AmnArora commented Jul 4, 2018

@pgarciacamou Firstly thanks for the quick reply. Well, after not finding anything on the web and posting the query here. I came up with the same solution you mentioned.

The Test cases are working now but this seems like a work around. I'll take a look at https://www.npmjs.com/package/react-context-consumer-hoc and discuss with my team.

Thanks. 💯

@AmnArora

This comment has been minimized.

Copy link

AmnArora commented Jul 4, 2018

@bvaughn The thing is earlier when I was using redux for state management, I shallow copied the Component and used dive() and instance() methods to get the instance of the component.

But none of these methods were available when using context API.

And when I wasn't using any of these methods I was getting the following error: unknown node with tag 12

@bvaughn

This comment has been minimized.

Copy link
Contributor

bvaughn commented Jul 4, 2018

Gotcha.

Both of these sound like issues with the version of Enzyme you're using not properly supporting the new context API. That's unfortunate.

@davalapar

This comment has been minimized.

Copy link

davalapar commented Dec 10, 2018

I got massive distaste for redux and unistore (a lot of code pollution / extra moving parts imo) which led me to the following setup which allows our nested components to access a single global state. Everything else that shouldn't be in the global state (ie. text input values, toggle values) are stored within the local state of each nested components.

https://github.com/davalapar/session-context

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment