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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use babel-plugin-transform-class-properties instead of bind #14

Merged
merged 4 commits into from Jul 8, 2018

Conversation

@bobheadxi
Copy link
Contributor

bobheadxi commented Jul 8, 2018

馃懛 Changes

See #11 (comment)

This PR replaces bind calls with some babel-plugin-transform-class-properties magic

馃挱 Notes

This uses the Stage 0 preset for babel-loader, which seems pretty broad but oh well

馃敠 Testing Instructions

make web
@bobheadxi bobheadxi added the :web label Jul 8, 2018
@bobheadxi bobheadxi requested review from d4l3k, chamkank and mingyokim Jul 8, 2018
@coveralls

This comment has been minimized.

Copy link

coveralls commented Jul 8, 2018

Pull Request Test Coverage Report for Build 143

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 0.0%

Totals Coverage Status
Change from base Build 133: 0.0%
Covered Lines: 0
Relevant Lines: 4

馃挍 - Coveralls
@bobheadxi bobheadxi force-pushed the web/transform-class-props branch from 649b433 to e0bad11 Jul 8, 2018
Copy link
Contributor

mingyokim left a comment

beauty 馃帀

@@ -75,7 +71,7 @@ class Login extends React.Component {
);
}

signedInView() {
signedInView = () => {

This comment has been minimized.

Copy link
@d4l3k

d4l3k Jul 8, 2018

Member

The view functions don't need to be arrow functions right?

This comment has been minimized.

Copy link
@bobheadxi

bobheadxi Jul 8, 2018

Author Contributor

well this one accesses this.props 馃し鈥嶁檪锔

This comment has been minimized.

Copy link
@d4l3k

d4l3k Jul 8, 2018

Member

Yes? this.props works just fine since signedInView is called via this.signedInView().

() => {} explicity binds this to the function. This is mostly a style issue really

This comment has been minimized.

Copy link
@d4l3k

d4l3k Jul 8, 2018

Member

There's some performance penalties associated with using foo = () => {} over foo(){} since the first doesn't end up in the prototype and is an object property instead.

https://medium.com/@charpeni/arrow-functions-in-class-properties-might-not-be-as-great-as-we-think-3b3551c440b1

Seems like @autoBind is the most performant way to do this overall. Though, the performance difference is probably negligible anyways.

This comment has been minimized.

Copy link
@bobheadxi

bobheadxi Jul 8, 2018

Author Contributor

oops yeah you're right about the view functions, I've fixed that 馃憤 thanks!

Though, the performance difference is probably negligible anyways.

馃挱

We should forget about small efficiencies, say about 97% of the time: premature optimization is the root of all evil.

@@ -19,10 +19,6 @@ class Login extends React.Component {

this.onEmailChange = event => this.setState({ email: event.target.value });
this.onPasswordChange = event => this.setState({ password: event.target.value });

This comment has been minimized.

Copy link
@d4l3k

d4l3k Jul 8, 2018

Member

Might consider moving these out of the constructor as well

@bobheadxi bobheadxi merged commit d585c90 into master Jul 8, 2018
5 checks passed
5 checks passed
Travis CI - Branch Build Passed
Details
Travis CI - Pull Request Build Passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 0.0%
Details
@bobheadxi bobheadxi deleted the web/transform-class-props branch Jul 8, 2018
@d4l3k

This comment has been minimized.

Copy link
Member

d4l3k commented Jul 8, 2018

If you're curious about what happens with all the different ways of doing function defs w/ binds here they are:

Source

import autobind from 'autobind-decorator'

class Component {
  constructor(value) {
    this.value = value

    this.constructorBind = () => {
      return this.value
    }
  }

  normal () {
    return this.value
  }

  arrow = () => {
    return this.value
  }

  @autobind
  bind() {
    return this.value
  }
}

Babel output

var _class;

function _defineProperty(obj, key, value) { if (key in obj) { Object.defineProperty(obj, key, { value: value, enumerable: true, configurable: true, writable: true }); } else { obj[key] = value; } return obj; }

function _applyDecoratedDescriptor(target, property, decorators, descriptor, context) { var desc = {}; Object['ke' + 'ys'](descriptor).forEach(function (key) { desc[key] = descriptor[key]; }); desc.enumerable = !!desc.enumerable; desc.configurable = !!desc.configurable; if ('value' in desc || desc.initializer) { desc.writable = true; } desc = decorators.slice().reverse().reduce(function (desc, decorator) { return decorator(target, property, desc) || desc; }, desc);if (context && desc.initializer !== void 0) { desc.value = desc.initializer ? desc.initializer.call(context) : void 0; desc.initializer = undefined; } if (desc.initializer === void 0) { Object['define' + 'Property'](target, property, desc); desc = null; } return desc; }

import autobind from 'autobind-decorator';
let Component = (_class = class Component {
  constructor(value) {
    _defineProperty(this, "arrow", () => {
      return this.value;
    });

    this.value = value;

    this.constructorBind = () => {
      return this.value;
    };
  }

  normal() {
    return this.value;
  }

  bind() {
    return this.value;
  }

}, (_applyDecoratedDescriptor(_class.prototype, "bind", [autobind], Object.getOwnPropertyDescriptor(_class.prototype, "bind"), _class.prototype)), _class);
@bobheadxi

This comment has been minimized.

Copy link
Contributor Author

bobheadxi commented Jul 8, 2018

ahhh yikes maybe autobind was the way to go, oh well

Thanks @d4l3k , this is all very insightful! will keep this in mind if we find the current method too slow

@d4l3k

This comment has been minimized.

Copy link
Member

d4l3k commented Jul 8, 2018

@bobheadxi

This comment has been minimized.

Copy link
Contributor Author

bobheadxi commented Jul 8, 2018

is the arrow function more or less doing the exact same thing as the constructor bind?

    _defineProperty(this, "arrow", () => {
      return this.value;
    });

    this.constructorBind = () => {
      return this.value;
    };
@d4l3k

This comment has been minimized.

Copy link
Member

d4l3k commented Jul 8, 2018

@bobheadxi bobheadxi mentioned this pull request Aug 11, 2018
@bobheadxi bobheadxi mentioned this pull request Sep 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can鈥檛 perform that action at this time.