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

Investigate what it would take to support function properties #242

Closed
gaearon opened this issue Apr 18, 2016 · 9 comments
Closed

Investigate what it would take to support function properties #242

gaearon opened this issue Apr 18, 2016 · 9 comments

Comments

@gaearon
Copy link
Owner

gaearon commented Apr 18, 2016

e.g.

class App extends Component {
  handleClick = () => {
    this.setState(...)
  }
}

One way to approach this would be to hoist the bodies of the function properties into hidden methods on the prototype with mangled names. This would be unobservable to the user but make them available for react-proxy to see and hot reload.

@nfcampos
Copy link
Collaborator

nfcampos commented Apr 19, 2016

hoist the bodies of the function properties into hidden methods on the prototype with mangled names

can you do that without a babel plugin?

@gaearon
Copy link
Owner Author

gaearon commented Apr 19, 2016

I don’t think so. But then again, you can’t have class properties without Babel anyway?

@nfcampos
Copy link
Collaborator

yeah but then aren't we back to needing to identify classes that look like react components at compile time?

@gaearon
Copy link
Owner Author

gaearon commented Apr 19, 2016

No, this transform is harmless for any class. I just mean turning

class Stuff {
  doSomething = (e) => {
    this.x(e.stuff())
  }
}

into

class Stuff {
  doSomething = (e) => {
    return this.___doSomethingqwertyu234567(e);
  },
  ___doSomethingqwertyu234567(e) {
    this.x(e.stuff())
  }
}

I’m not sure if there is anything in the spec that would make it a bad idea but it’s worth giving it a shot. The only special case I see so far is arguments object which should still end up being the one in constructor. To be honest we can bail completely when we see arguments used.

@nfcampos
Copy link
Collaborator

nfcampos commented Apr 19, 2016

Hmm, I see, we might hit some edge cases related to the differences between regular functions and arrow functions, so we should bail out on use of super, new.target, and arguments as you said, and we also need to convert arrow functions without curly braces to regular functions with body

class Stuff {
  doSomething = (e) => this.x(e.stuff())
}
class Stuff {
  doSomething = (e) => {
    return this.___doSomethingqwertyu234567(e);
  },
  ___doSomethingqwertyu234567(e) {
    return this.x(e.stuff())
  }
}

so maybe it might be better to translate it to

class Stuff {
  doSomething = (e) => {
    return this.___doSomethingqwertyu234567(e);
  },
  ___doSomethingqwertyu234567(e) {
    return ((e) => this.x(e.stuff()))(e)
  }
}

this way the function would still be an arrow function... on the other hand we're creating a function on every invocation...

@gaearon
Copy link
Owner Author

gaearon commented Apr 19, 2016

We need to create it once, otherwise it’s slow.

I’m not really sure I see your point about functions without body. We can just convert them to arrow functions without body that call those hidden methods. Or we can bail out because most event handlers are more than one liners, and commonly React code uses those just for event handlers. Handling majority of cases is good enough here.

@nfcampos
Copy link
Collaborator

yeah you're right, this doesn't need to work for everything.
I guess I'm just afraid of anything that changes code behaviour between development (where you'd use react hot loader, and class properties would be converted in this fashion) and production where you don't use it, especially if it changes the behaviour in subtle ways, like the differences between an arrow function and a regular function.

@gaearon
Copy link
Owner Author

gaearon commented Apr 19, 2016

Yeah I get it, but if Babel is able to compile arrow functions correctly, so can we 😄 . Just a matter of having enough tests.

@calesce
Copy link
Collaborator

calesce commented Sep 15, 2016

Added by #322, and out in 3.0.0-beta.4, thanks @nfcampos!

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

No branches or pull requests

3 participants