Skip to content

this is undefined in getDerivedStateFromProps #14730

Closed
@sanbornhilland

Description

@sanbornhilland

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);

Activity

TrySound

TrySound commented on Jan 30, 2019

@TrySound
Contributor
milesj

milesj commented on Jan 30, 2019

@milesj
Contributor

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

sanbornhilland commented on Jan 30, 2019

@sanbornhilland
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

jquense commented on Jan 30, 2019

@jquense
Contributor

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

sanbornhilland

sanbornhilland commented on Jan 30, 2019

@sanbornhilland
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

milesj commented on Jan 30, 2019

@milesj
Contributor

@sanbornhilland That's how JS works.

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

milesj

milesj commented on Jan 30, 2019

@milesj
Contributor

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

sanbornhilland commented on Jan 30, 2019

@sanbornhilland
Author

@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

jquense commented on Jan 30, 2019

@jquense
Contributor

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

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

milesj commented on Jan 30, 2019

@milesj
Contributor

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

mizukami234

mizukami234 commented on Dec 5, 2019

@mizukami234

This works (not recommended though):

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

dolsem commented on Aug 5, 2020

@dolsem

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

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

      Development

      No branches or pull requests

        Participants

        @milesj@jquense@mizukami234@TrySound@sanbornhilland

        Issue actions

          this is undefined in getDerivedStateFromProps · Issue #14730 · facebook/react