Binds To Global

[thehoglet]

Problem here?

I think this might need to be bound, i.e. change line to

setTimeout(this.drawFauxDOM.bind(this))

---

Thanks for the lib.

[Olical]

Probably a safe move although it doesn't appear to be causing any issues right now which I find strange.

[tibotiber]

Hi @Olical! Nice lib, thanks.

I'm getting a warning each time I connectFauxDOM which seems to come from this fix. Removing the binding to the component gets rid of the warning.

Warning: bind(): You are binding a component method to the component. React does this for you automatically in a high-performance way, so you can safely remove this call. See BarChart

[tibotiber]

@Olical
additional detail: I'm using a React.createClass component as per your example with mixins, which is why the method is bound to the component by React. Not sure if you are trying to support mixins for ES6 class components? or if it is even possible? Maybe moving to an higher-order component would be more in line with the latest best practices?

[Olical]

Well, since this wasn't a requirement in the first place (I think it was just a hunch that I didn't really look into / didn't think could hurt) I can revert it.

I didn't add these mixins and I don't fully understand them I'm afraid, it was an addition by a contributor. I know React is phasing mixins out in favour of higher order components (I think?), so these probably only support the "older" way of doing things.

[tibotiber]

Thanks for the clarification.

I think reverting the binding would be a good choice. @thehoglet any objection I might miss?

If I find some time, I might look into updating the mixins to higher-order components. It was probably the best approach at the time they were added but if I'm not wrong it's being phased out with ES6 classes. I've been wanting to learn about higher-order components anyway, so if it can be useful also :).

[Olical]

Yep, there was no error / hard reason to add it in the first place, it just looked right.

If you wanted to convert them, that'd be awesome. We're on a level playing field with that part of the project since all I've really done is reviewed and merged it since it was entirely optional for end users.

I still hold to "use this for very simple things, drop to real D3 for animations and complex things", but kept the mixins because they probably solve problems for some people. No pressure though, I'll happily review it if you do have a play with it.

[thehoglet]

@tibotiber: no objection.

I was using TypeScript at the time and I suspect the way I was using it to implement my component classes was affecting React's default binding behaviour.

I did not use the mixin as I could not figure out a clean way to incorporate it in a TypeScript class. I'm new to React HOCs so will be interested to see what you come up with.

[tibotiber]

@thehoglet thanks for the feedback :)
@Olical I think we're good to go with the revert