reflux / refluxjs

A simple library for uni-directional dataflow application architecture with React extensions inspired by Flux
BSD 3-Clause "New" or "Revised" License
5.36k stars 330 forks source link

componentWillMount and Reflux this.store don't play nice together. #499

Closed eek closed 7 years ago

eek commented 7 years ago

I have an issue, using the ES6 Classes, if I have componentWillMount on my React Component, then the component will not listen to the Store. Can we make both of them work?

Or at least show an error in case the Component doesn't link to the Store.

I didn't understood why the Component did not update with the Store's state and eventually found out that if I have an componentWillMount, the Component will not be linked with the Store.

jamesmarrs commented 7 years ago

@eek I started reworking some of my old code last week to implement the new es6 features that @BryanGrezeszak was so nice to work on. I ran into some similar problems. If you have componentWillMount and componentWillUnmount in your Reflux.Component you need to add this line inside the function to make sure everything gets bound correctly:

// in your componentWillMount() 
Reflux.Component.prototype.componentWillMount.call(this);
// in your componentWillUnmount()
Reflux.Component.prototype.componentWillUnmount.call(this);

After the new docs were out I was going to check them over to make sure this got in there somewhere, as its easy to miss!

BryanGrezeszak commented 7 years ago

Yep, this is something that's going to be in the new docs. Since the ES6 classes need to utilize some of the React lifecycle it is possibly to overwrite what they use. componentWillMount and componentWillUnmount are the only 2 it uses and therefore the only 2 possible issues.

The only real question about it is: do we need an API call for this, or (since it's a one-liner anyway) leave it to the raw JS way?

Of course, if the friggin ECMAScript guys would just stop tiptoeing around the concept of classes poking one toe in at a time and would instead just COMMIT and jump in all the way then they would just let us super() any method (instead of just the constructor) like every other language out there, and the question would be solved (...yes, I'm bitter about these sort of things).

Something like this.manualWillMount() and this.manualWillUnmount() or something like that offers a little magic to do it...and I wouldn't feel too bad about that since it's a more obscure issue anyway, so it wouldn't clutter up the docs too bad. On the other hand...it's just a one liner in plain JS...just documenting it well enough could work. I'm torn on this one.

And @jamesmarrs thanks for the help in handling the issue so quickly for him.

FlorianWendelborn commented 7 years ago

@BryanGrezeszak You have to consider that right now it's just a single, easy line of javascript. In the future that might change.

Therefore I'd vote for a small, documented function.

BryanGrezeszak commented 7 years ago

@dodekeract - I'm leaning that direction too. Not so much for that reason (it's a line that's pretty fundamental to how JS works with prototypes, so I'm not worried about that). I just plain think it's too verbose to ask people to remember. I'm one of the people telling people how to do it, yet even I typed it incorrectly on a few occasions and was left wondering why my stuff wasn't mounting!

I'll have to put some thought into naming though. So we'll see where it goes.

tommytroylin commented 7 years ago

@BryanGrezeszak @dodekeract

super.componentWillMount.call(this); in componentWillMount would be ok. I vote for just documenting it.

Now users know that writing this.store = store; in constructor in order to connect a store. So let them know add a line in these two functions to enable it correctly won't take them much more efforts to understand.

Just provide a example with something like

componentWillMount() {
    super.componentWillMount.call(this);
   // your componentWillMount code goes here
  }
...

Since Reflux.Component is an enhancement of React.Component and Reflux encourages user to replace React.Component by Reflux.Component. Adding some renamed functions and asking users to move their code into them makes a bit more confusing.

BryanGrezeszak commented 7 years ago

@tommytroylin - Where is that super object coming from? JS only has super usage (and it's as a function) in constructors.

tommytroylin commented 7 years ago

@BryanGrezeszak developer.mozilla.org

BryanGrezeszak commented 7 years ago

@tommytroylin -

I've seen that in the spec, yet every time I've ever console logged this super object to see if it's implemented yet, it errors.

Did it just now in babel, and on codepen and both errored.

And when I researched the topic of super usage outside of constructors I found an entire article about how the feature doesn't exist, and all sorts of ways around it.

Yet MDN says its implemented. Am I going crazy?

BryanGrezeszak commented 7 years ago

...ok, I'm going crazy a little bit.

Apparently if you try to access the super object itself...error.

But if you access properties of it...you're fine.

I've literally been checking for that feature on and off for probably at least a year now...each time thought it wasn't working because accessing the super object failed. Didn't expect that you'd be allowed to access properties on it without being allowed to access it itself.

Oh well, a feature is a feature. I'll take it. Makes a lot more sense than implementing our own method.

tommytroylin commented 7 years ago

@BryanGrezeszak ECMAScript Spec - The super keyword shows that super is a keyword not an object. So assign/capture it as an object will cause error.

From my tests, when using super['propertyName'] or super.propertyName as a getter, it actually refer to SuperClass.prototype.propertyName.

When you call it as a function like super.sayHello(), it actually implicit called withthisas same as SuperClass.prototype.sayHello.call(this)

Back to this issue, it can be simplified to super.componentWillMount(). But I've not checked if it works well yet...

BryanGrezeszak commented 7 years ago

this is a keyword too, but I can still log the object it references at any given moment. I expected similar behavior from super.

Since I couldn't access it I just thought that meant the feature wasn't actually implemented. Oh well, live and learn: JS always has a surprise in store, no matter how much you think you know :)

But yes, it should remove the need for any call. So that's even better.

BryanGrezeszak commented 7 years ago

Documented.