Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Will this convention be supported? #427

Closed
Rokt33r opened this issue Nov 24, 2016 · 8 comments · Fixed by #464
Closed

Will this convention be supported? #427

Rokt33r opened this issue Nov 24, 2016 · 8 comments · Fixed by #464

Comments

@Rokt33r
Copy link
Contributor

Rokt33r commented Nov 24, 2016

I don't want to use this.handleSliderMouseDown = this.handleSliderMouseDown.bind(this) because this is too verbose. Also I don't want to apply the experimental syntax, property-initializers.

So, I've been binding function by declaring an arrow function inside of constructor.

 constructor (props) {
    super(props)

    this.state = {
      listWidth: props.status.get('noteListWidth')
    }

    this.handleSliderMouseDown = e => {
      window.addEventListener('mouseup', this.handleSliderMouseUp)
      window.addEventListener('mousemove', this.handleSliderMouseMove)
      this.setState({
        isSliderActive: true
      })
    }
}

It works well as long as i use only HMR without React hot loader.
If I use HMR, instance methods aren't refreshed because constructor won't be fired again.

Can I use this syntax and wait the fix? Or should I stop using it?

@calesce
Copy link
Collaborator

calesce commented Nov 25, 2016

property-initializers (or class properties) are basically sugar for what you're doing, see here.

The problem is that React Hot Loader can't hot-reload the constructor without the component being re-instantiated (and remounting/losing its state). It can reload class methods, though, so part of our Babel plugin actually deals with this by copying the class property bodies to class methods, see this discussion.

We could probably do the exact same transform to support this pattern. I don't have much time to implement this right now, but I'd be happy to give some pointers to someone who wants to try it. 😄

@Rokt33r
Copy link
Contributor Author

Rokt33r commented Nov 26, 2016

I want to try. Where should I start?

@calesce
Copy link
Collaborator

calesce commented Nov 27, 2016

First, I recommend reading through babel-handbook if you haven't already, it's amazing. Also, AST Explorer is really useful for understanding JS ASTs. Also, babel-types is your best friend for creating new AST nodes.

The best way to understand what the Babel plugin does is to look at the test fixtures, for expected input/output. It's probably best to create more fixtures in this style as test cases for the new behavior.

This part of the plugin deals with finding class properties, creating new methods, and creating the class property that just proxies to the new method.

You can probably look for (arrow) functions defined on this in the constructor, and do the same type of manual replacement using path.replaceWith and creating a new class method the same way.

Hope that's helpful!

@Rokt33r
Copy link
Contributor Author

Rokt33r commented Nov 28, 2016

This is what I need. Thank you so much! I'll give a shot soon.

@paulpooch
Copy link

I've been doing the method prescribed in this link and having an issue with alias pollution. Every reload ceates an alias for every class method (bound handler).

http://reactkungfu.com/2015/07/why-and-how-to-bind-methods-in-your-react-component-classes/

So basically:

class CatPicker extends Component {

  handleClick = (e) => {
    // do stuff.
  }
 
  render() { ... }
}

With the downside being hot loader creates aliases every time it reloads. So you end up with this grossness:


}, {
    key: '__handleTimeSelected__REACT_HOT_LOADER__',
    value: function __handleTimeSelected__REACT_HOT_LOADER__() {
      return this.__handleTimeSelected__REACT_HOT_LOADER__.apply(this, arguments);
    }
  }, {
    key: '__handleTimeSelected__REACT_HOT_LOADER__',
    value: function __handleTimeSelected__REACT_HOT_LOADER__() {
      return this.__handleTimeSelected__REACT_HOT_LOADER__.apply(this, arguments);
    }
  }, {
    key: '__handleTimeSelected__REACT_HOT_LOADER__',
    value: function __handleTimeSelected__REACT_HOT_LOADER__() {
      return this.__handleTimeSelected__REACT_HOT_LOADER__.apply(this, arguments);
    }
  }, {
    key: '__handleTimeSelected__REACT_HOT_LOADER__',
    value: function __handleTimeSelected__REACT_HOT_LOADER__(e) {
      // const appointmentDate = moment().format(DATE_PARAM_FORMAT);
      // this.props.selectAppointmentDate({ appointmentDate });
    }
  }, {
    key: 'render',
    value: function render() {

@calesce
Copy link
Collaborator

calesce commented Dec 7, 2016

@paulpooch you're talking about something different from the original issue. can you move that comment to a new issue please? I don't want the prior discussion to get sidetracked with a separate topic.

@Rokt33r
Copy link
Contributor Author

Rokt33r commented Jan 19, 2017

@calesce
I need some help... I've confronted two problems.

How to visit AssignmentExpression inside of constructor?

I found the way to find constructor. but I have to check every AssignmentExpression.

I'm now can parse this style just by iterating ExpressionStatement.

class Foo {
  constructor () {
    this.onClick = (e) => e.target.value
  }
}

But, I can not parse the following... because I have to check all children...

class Foo {
  constructor () {
    if (true) {
      this.onClick = (e) => e.target.value
    }
  }
}

How should I write the test?

Does it look good? Also, how should I check it actually work?

// actual.js
class Foo {
  constructor () {
    this.onClick = (e) => e.target.value
  }
}

// expected.js
class Foo {
  constructor() {
    this.onClick = (...params) => this.__onClick__REACT_HOT_LOADER__(...params);
  }

  __onClick__REACT_HOT_LOADER__(e) {
    return e.target.value;
  }

}
;

var _temp = function () {
  if (typeof __REACT_HOT_LOADER__ === 'undefined') {
    return;
  }

  __REACT_HOT_LOADER__.register(Foo, "Foo", __FILENAME__);
}();

;

@Rokt33r
Copy link
Contributor Author

Rokt33r commented Jan 19, 2017

I think I found the solution of the first question.

{
  ClassMethod (path) {
    path.traverse({
      AssignmentExpression(path) {
        // ...
      }
    });
  }
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants