Skip to content
This repository was archived by the owner on Jun 24, 2021. It is now read-only.

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

Merged
bobheadxi merged 4 commits into
masterfrom
web/transform-class-props
Jul 8, 2018
Merged

Use babel-plugin-transform-class-properties instead of bind#14
bobheadxi merged 4 commits into
masterfrom
web/transform-class-props

Conversation

@bobheadxi

@bobheadxi bobheadxi commented Jul 8, 2018

Copy link
Copy Markdown
Contributor

👷 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 chamkank, d4l3k and mingyokim July 8, 2018 02:28
@coveralls

coveralls commented Jul 8, 2018

Copy link
Copy Markdown

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 Compare July 8, 2018 02:34

@mingyokim mingyokim left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

beauty 🎉

Comment thread web/components/login/Login.js Outdated
}

signedInView() {
signedInView = () => {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@bobheadxi bobheadxi Jul 8, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

well this one accesses this.props 🤷‍♂️

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread web/components/login/Login.js Outdated
@@ -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 });

@d4l3k d4l3k Jul 8, 2018

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might consider moving these out of the constructor as well

@bobheadxi bobheadxi merged commit d585c90 into master Jul 8, 2018
@bobheadxi bobheadxi deleted the web/transform-class-props branch July 8, 2018 04:29
@d4l3k

d4l3k commented Jul 8, 2018

Copy link
Copy Markdown

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

Copy link
Copy Markdown
Contributor Author

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

d4l3k commented Jul 8, 2018 via email

Copy link
Copy Markdown

@bobheadxi

bobheadxi commented Jul 8, 2018

Copy link
Copy Markdown
Contributor Author

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

d4l3k commented Jul 8, 2018 via email

Copy link
Copy Markdown

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants