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

this is undefined in getDerivedStateFromProps #14730

Closed
sanbornhilland opened this issue Jan 30, 2019 · 12 comments
Closed

this is undefined in getDerivedStateFromProps #14730

sanbornhilland opened this issue Jan 30, 2019 · 12 comments

Comments

@sanbornhilland
Copy link

sanbornhilland commented Jan 30, 2019

Do you want to request a feature or report a bug?
Bug

What is the current behavior?
this is undefined in getDerivedStateFromProps

If the current behavior is a bug, please provide the steps to reproduce and if possible a minimal demo of the problem. Your bug will get fixed much faster if we can run your code and it doesn't have dependencies other than React. Paste the link to your JSFiddle (https://jsfiddle.net/Luktwrdm/) or CodeSandbox (https://codesandbox.io/s/new) example below:

class MyComponent extends Component {
  static sayHello() {
     console.log('Hello')
  }

  static getDerivedStateFromProps() {
     this.sayHello()
  }
}

What is the expected behavior?
I would expect the above to work. To me it is unexpected that a static method is not getting called on the class.

Which versions of React, and which browser / OS are affected by this issue? Did this work in previous versions of React?
16.5.1

This seems like an easy fix to me, here

Is there a reason not to change line 157 and 161?

// From this
getDerivedStateFromProps.(nextProps, prevState);

// To this?
getDerivedStateFromProps.call(ctor, nextProps, prevState);
@TrySound
Copy link
Contributor

@sanbornhilland This is expected. #12612 (comment)

@milesj
Copy link
Contributor

milesj commented Jan 30, 2019

To me it is unexpected that a static method is not getting called on the class.

Outside of what @TrySound has linked, this is how JS works. Static methods do not have access to a class instance's this, and IMO, it never should.

@sanbornhilland
Copy link
Author

It's not even null though, which would indicate that it was intentionally set. It's undefined which looks like a bug to me.

I understand wanting to avoid unnecessary confusion in the React api but this misses the mark IMO. To me this is unexpected behaviour and at some point it seems reasonable to encourage developers to learn how this works instead of being overly protective.

@jquense
Copy link
Contributor

jquense commented Jan 30, 2019

this is intended, and yes it does differ from how static methods, work if you call them with the class to the left of the dot Foo.bar(). For the reasons noted in the other issue The react team has opted to avoid ambiguity and always make sure it's undefined

@jquense jquense closed this as completed Jan 30, 2019
@sanbornhilland
Copy link
Author

@milesj I'm not sure what you mean by

Static methods do not have access to a class instance's this, and IMO, it never should.

That's demonstrably false so maybe I'm misunderstanding your point.

@milesj
Copy link
Contributor

milesj commented Jan 30, 2019

@sanbornhilland That's how JS works.

screen shot 2019-01-30 at 10 20 43 am

@milesj
Copy link
Contributor

milesj commented Jan 30, 2019

You can see their usage here:

const getDerivedStateFromProps = ctor.getDerivedStateFromProps;

They are simply calling it as a reference. No bind/call/apply, etc. This is standard JS classes.

@sanbornhilland
Copy link
Author

sanbornhilland commented Jan 30, 2019

@milesj Sorry mate, don't want to turn this into a debate but you're misreading your console output. Your example code demonstrates that this is defined in that case because you are logging function Foo(). The undefined you are seeing is the return value of console.log.

Appreciate the link to the code but that just proves the point I was making. When you assign the function like that then you lose the context of ctor which is why I was advocating for using call, to avoid losing that context.

EDIT: Just to be clear. I think I understand the confusion now. Perhaps you thought I was asking for this to be bound to an instance. Of course that doesn't make sense. As per the original issue, I was asking that static methods have this referencing the class.

Anyway, I see that this is a deliberate decision by the React team so that's fine. I appreciate the quick response, all.

@jquense
Copy link
Contributor

jquense commented Jan 30, 2019

this in a static method can be the constructor (not the instance)

class Foo { static bar() { console.log(this.baz) } static baz(){} }

@milesj
Copy link
Contributor

milesj commented Jan 30, 2019

@sanbornhilland Correct, I was assuming you wanted the instance, not the constructor. Definitely clears up the confusion.

@mizukami234
Copy link

This works (not recommended though):

static get getDerivedStateFromProps() {
  return (props, state) => {
    console.log("Hello this is ", this);
  };
}

@dolsem
Copy link

dolsem commented Aug 5, 2020

This issue should be reopened because this is expected to point to the class instance. See this contrived example. TypeScript typechecker is fine with this code because it expects this to be defined. This code compiles but fails at runtime. This becomes more of an issue when you create a component intended to be subclassable and so you don't know the exact context beforehand, like in the example above. I have to work around it with a getter like @mizukami234 suggested.

  static get getDerivedStateFromProps() {
    return this._getDerivedStateFromProps.bind(this);
  }

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