React: Context of `getDerivedStateFromProps` is null

[timdegroote]

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

A bug

What is the current behavior?

When getDerivedStateFromProps is called the context in which it is executed is null, making it impossible to access other statically defined class methods through this.

JSFiddle: https://jsfiddle.net/Luktwrdm/390/

What is the expected behavior?

I would expect the provided context to be the class in which the method is defined. Example of a simple class in which other static methods are accessed: https://jsfiddle.net/33nc7jhg/3/.

[jquense]

See my comment in the PR. You cant access static methods on a class via this anyway, since they are defined on the constructor,not the instance. If you do want to access static methods in gDSFP you can do so like App.otherStaticMethod()

[timdegroote]

Hey @jquense, thanks for your reply!

I'm not sure however if we're aligned on the issue here. My intention wasn't to pass the instance as the context to gDSFP but the actual constructor. One should be able to access other static methods from a static method (see https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Classes/static#From_another_static_method).

The usual context within static class methods is thus the constructor (indeed not the instance). However in gDSFP the context is explicitly set to null. In the associated PR the context will be the constructor, thus any instance methods/properties are still unavailable.

[jquense]

oops sorry @timdegroote reviewed to quickly. I'm still not sure whether this should be considered a bug, my guess is the team explicitly chose the null context to not lead to "why is this weird" questions but that's just a speculation.

I'll open it all back up

[gaearon]

my guess is the team explicitly chose the null context to not lead to "why is this weird" questions but that's just a speculation.

Yeah, at least from my point of view the intention is to highlight that this won’t be the instance and to discourage people from writing code that uses it here. This is the only static lifecycle method that we have, and it’s just way too easy to get confused already. Allowing this in it compounds the problem for future readers.

I understand this might seem a bit overprotective. But I don’t see practical upsides to allowing it. And I see downsides: this will be there, it will just point to a different thing, and confuse anyone who doesn’t know JS deeply. For example I’d expect people to declare class properties and then try to access them (which crashes now but would give you undefined with this change).

This convention isn’t even a good shorthand convenience. Because if you want to make it shorter you can hoist it in a constant or a module level variable or function. It’s both easier to reference and more explicit. It’s also better encapsulated because other modules can’t access it.

[timdegroote]

The way I ran into this was as following: I had a use case for gDSFP and wanted to try it out. After reading the documentation I realised I would need some methods that were initially on the instance to be ran on certain props so I started converting them to static methods (I agree this isn't the best solution, but this was so far just a test anyway). Then I ran into the issue that I couldn't access my static methods from the gDSFP method.

I initially thought my understanding of static methods was incorrect. After confirming that this wasn't the case I thought maybe something was wrong with my babel/webpack setup, so I spent some time there making sure the proper plugins etc were installed and configured. It wasn't until I finally had a look into how the gDSFP method was called from React that I understood what the issue was.

Just adding this to the conversation to point out that for users that might expect the context to be consistent with the language spec it will also be confusing. If they don't get down to the actual cause of the problem they might suspect something is wrong with their setup or get the wrong idea of how static methods work.

In the end I agree with your opinion and thought process. I can definitely see where you're coming from and understand the decision. Thank you for the clarification!

[gaearon]

In JS the context is determined by the callsite so I wouldn’t say it’s “inconsistent with the spec”. I do agree it can be confusing when you’re already familiar with this pattern, or if you’ve read about it while searching for a solution.

In my anecdotal experience people coming to JS from other languages tend to first try MyComponent.methodName() to call a static method which would work. If this.methodName() was the only way to do it I think your concern would prevail, but in this case I think it’s not that common, so we’d rather sacrifice some confusion here than elsewhere.

Here, the confusion is explicit. You know it just didn’t work. You researched the problem, got an answer, and filed an issue. Somebody else will find it later, post it to StackOverflow, and eventually it will become common knowledge. The important part is that you knew there was a problem.

The far more unpleasant case is when you don’t know you have a problem. Such as with quietly undefined fields.

Or where you can’t identify the problem. It’s easier to identify this being null. It’s harder to say what exactly is wrong when the code already successfully uses this but can’t use this.props for some reason. It wouldn’t occur to many people that this may exist but point to something other than the instance.

So I think overall even though both approaches have problems, I’d prefer a problem that “fails fast” over the one that can subtly hide a misunderstanding. Even if the second one might be more conventional in some JS coding styles.

[sznowicki]

I don't like this decision. I took me too much time checking why "this" was null. I even started to think that maybe it's just in my head that static methods can call each other since "this" is the context of constructor/prototype.

IMHO the proposed change should be merged since this is what javascript developer should expect.

The intention to avoid confusion in people coming form other than JS languages in my opinion might lead to more confusion in the future.

Also:
Using MyComponent.methodName also disables any inheritance possibilities (to e.x. exchange some static methods with another ones). It will always call the "hardcoded" one.

I know inheritance is discouraged in the community, but some developers use it for some approaches and without proposed change some things might be harder to achieve.

[wuxyman]

Actually,i don`t konw how to use getDerivedStateFromProps;the method cannot stop a class life cycle ,even thought it return null.