-
Notifications
You must be signed in to change notification settings - Fork 263
@autobind doesn't work correctly with react-hot-loader <= 1.3.0 #48
Comments
Works for me: http://react.jsbin.com/baqemeqedi/edit?js,output Guessing your issue is elsewhere. Can you be more specific when you say it doesn't work? What happens instead? |
@wmertens ping |
Yes, sorry, need some more time to repro. The problem is that I am trying Wout. |
Hey there. I'm also having this problem. For me, the issue occurs here: https://github.com/jayphelps/core-decorators.js/blob/master/src/autobind.js#L68, wherein import React, { PropTypes, Component } from 'react'
import { autobind } from 'core-decorators'
import { Button } from './custom-button-component'
export class ActivityCalendar extends Component {
@autobind
toggleCalendar () {
// `this` is null when called
this.setState({ showCalendar: true })
}
render () {
return (
<div>
<Button cssClass='btn--input'
onClick={this.toggleCalendar}>
<span className='icon'></span>
</Button>
</div>
)
}
} The <button
type={ this.props.type }
onClick={ this.props.onClick }
className={'btn ' + this.props.cssClass }>
{ this.props.children }
</button> core-decorators: 0.11 Let me know if you need any more info or I can try something. |
@mike-engel your example, as provided, works for me as well. http://react.jsbin.com/dakayo/3/edit?js,output
I'm gonna create a local project with the same jsbin code using npm instead of globals just in case (done, see next comment). In the meantime can you confirm the provided example as-is reproduces the issue for you locally, without additional code that may have been omitted? Edit: I'm absolutely not dismissing you btw. Clearly there is a problem somewhere. I'm just inclined to believe it's somehow related to a conflict between core-decorators and some other library or code that hasn't been presented to me yet. Our tests are pretty exhaustive and our |
@mike-engel Here's a ready-to-run app that uses your example code with some very minor modifications so you can actually see feedback that it's working. https://github.com/jayphelps/core-decorators-issue-48 $ git clone https://github.com/jayphelps/core-decorators-issue-48.git
$ cd core-decorators-issue-48
$ npm install
$ npm start
$ # now open localhost:7001 in your browser It works as expected. ¯_(ツ)_/¯ any guesses on your end? |
Also of note, I could not install babel
|
Thanks for putting that together @jayphelps! I'm running it now. Sorry for the babel typo, I'm on |
Also, FWIW, downgrading to core-decorators |
@mike-engel what browser/version are you using? |
Original breakage reported on Firefox 44. The demo, however, is working on both FF 44, 45, and Chrome stable. |
@mike-engel In the file causing you trouble, can you remove the import of function autobind(target, key, { value: fn }) {
return {
configurable: true,
enumerable: false,
get() {
const boundFn = fn.bind(this);
Object.defineProperty(this, key, {
configurable: true,
writable: true,
// NOT enumerable when it's a bound method
enumerable: false,
value: boundFn
});
return boundFn;
},
set(newValue) {
Object.defineProperty(this, key, {
configurable: true,
writable: true,
// IS enumerable when reassigned by the outside word
enumerable: true,
value: newValue
});
return newValue;
}
};
} |
Quick update @jayphelps: I haven't had a lot of time yet to continue looking into slowly adding code until I find the piece that breaks. I did get a couple minutes, however, to use the simple autobind function. It does bind @autobind
toggleCalendar () {
console.log(this) // react component object
console.log(this.props) // undefined
} I know I should have at least some props since I have |
@mike-engel inside @autobind
toggleCalendar () {
console.log(this === ActivityCalendar, this instanceof ActivityCalendar);
} I have a feeling it isn't an instance of |
@mike-engel are you calling or accessing the property |
@jayphelps Results from the log are return (
<div className={getTaskClassName('activity')}>
<TaskSummary task={this.props.task}
iconClassName={getIconClassName(this.props.taskState.showTaskContent)}
handleClick={this.handleExpandContent} />
<Collapse isOpen={this.props.taskState.showTaskContent}>
<div className='activities-detail'>
{this.renderActivityHeader()}
<div className='activities-detail__body'>
<span className={classnames(reportClasses)} onClick={this.toggleSelfReport}>Report activity <span className='ico-caret-next'></span></span>
<Collapse isOpen={this.props.taskState.showReportForm}>
<ActivityLogger task={this.props.task}
taskState={this.props.taskState}
logEntries={this.props.log}
dispatch={this.props.dispatch} />
</Collapse>
</div>
</div>
</Collapse>
</div>
) If you're wondering,
Just for good measure, I threw this up at the top-most react component/entry point of our app @autobind
handleButtonClick () {
console.log(this.props) // `this` is null
}
render () {
return (
<div>
<button onClick={this.handleButtonClick}>Test click</button
...other element stuff...
</div>
)
} This is a code base I just jumped on, so I'm still trying to figure out what's different in this one compared to ones I've used before (and where core-decorators work splendidly). As I get time, I'm going to keep digging like I mentioned earlier. |
@mike-engel I don't see |
@mike-engel I'm happy to do a Screenhero session with you to try and really debug this in your actual codebase. I realize your company may have issues with outsiders seeing the code, but perhaps just one person might ease that concern. Happy to esign an NDA (most companies have "hired" contractors so they can treat me like one, just no need to pay me). I really want to get to the bottom of this in case this is truly something core-decorators can prevent others from hitting. |
@jayphelps Sorry about that—multiple people working in the codebase === more abstractions sometimes 😄 But yes, it used to be If I can't pintpoint the exact location/component/module causing this to break, I'll check around with the higher ups™ to see if I can't run through it with you. I'll try and update you by the end of the week if that's ok? |
@mike-engel no rush for me, it's your problem |
@mike-engel although you didn't mention using |
@jayphelps No luck 😦 I did get some time tonight to play around, and found a couple things.
This leads me to believe there's some webpack loader or something breaking things, and I'll try to look at it this week(end). Thanks for the heads up, and sorry for the silence. |
@mike-engel if it works only with prod builds it very much suggests a babel plugin injecting things for debug, just like #56 was (but probably a different one than #56 since Can you list out the babel plugins you're using? Most likely one of the various react-transforms that are popular with redux folks. Actually, just paste in all your devDependencies 😄 |
@wmertens still seeing this issue with |
@jayphelps, here are all our devDependencies:
I already tried removing the babel rewire plugin from out config and that didn't seem to be the issue. Let me know if you'd like me to try anything. |
Pulling in @gaearon so he can confirm my findings and suggestions. @mike-engel Thank you for providing your deps, found your issue using them and I'm betting it's the same one @wmertens has since it is with react-hot-loader's @wmertens can you read this as well and see if it applies to you? (if you use It was fixed here but basically it is the way they are looking up and invoking the end method, basically proxying but doing so in a brittle way that loses the react-hot-loader appears to be deprecated but there was some work that includes the version of SO...in summary, there isn't anything I'm aware of that I can do to mitigate this on my end because we need to support looking up the original property without autobind coming along. i.e. You have a couple options I think:
If someone can think of a way for |
@jayphelps First off, thanks so much for your time and effort looking into this! This was a project I inherited so apologies for not being more helpful 😅 Second, I'm totally ok with either updating react-hot-loader or moving to a more modern implementation of hot loading react code. No need to worry about this one edge case, especially if it's with a deprecated third party module. As far as I'm concerned, this issue can be closed and I'll work independently on getting our deps updated (fat arrow functions are working for now). Thanks again for the help, and thanks for publishing this module. I've found it super useful and I know my team has too. |
@jayphelps wow, thanks for finding this! I will try your solutions today and report back. Sorry I have not been more responsive, swamped with work :-( |
I'm using |
@raphaelvigee They released 1.3.1 since this thread was originally created, but it did not include the described fix they needed. They have also deprecated react-hot-loader. I'm not aware of a way for core-decorators to workaround this--remember, this is due to the way react-hot-loader is patching methods (technically their dependency). Wish I could help, but don't know how I can. |
I just tested this with the react hot loader 3 beta and I can confirm that it is solved there. @jayphelps since this really not core-decorator's issue, shall I (or you) close this? Or would you like to keep it open for visibility? |
I am using @autoBind together with React and Redux. It works in several classes, but I have one class that it doesn't work with. I tried eliminating a bunch of things like
@connect
but I can't make it work.Here's an example of how I use it, but this does work:
Can you think of anything that would make the autobind fail? Hot reloading, other decorators, bugs in Babel? What should a debug trace look like?
The text was updated successfully, but these errors were encountered: