developit / decko

:dash: The 3 most useful ES7 decorators: bind, debounce and memoize
https://developit.github.io/decko/
MIT License
1.03k stars 36 forks source link

@bind() decorator not working in IE11 #8

Closed davidrus closed 6 years ago

davidrus commented 7 years ago

I have this small example:

import { Component, h, define } from 'skatejs';

import { bind } from 'decko';

export default class App extends Component<void> {
  static get is() { return 'my-app'; }

  @bind()
  handleClick() {
    console.log( 'clicked with this scope: ', this );
  }

  renderCallback() {
    return [
      <div onClick={this.handleClick}>Hello my lord!</div>,
    ];
  }
};

define(App);

When I try this in IE (11) I have this error: image

In chrome it works properly...

You can try it in this repository which I created for this issue: https://github.com/davidrus/skate-starter

developit commented 7 years ago

@davidrus Seems like you're adding parens to the decorator, which is maybe causing the issue. It should look like:

import { Component, h, define } from 'skatejs';

import { bind } from 'decko';

export default class App extends Component<void> {
  static get is() { return 'my-app'; }

  @bind
  handleClick() {
    console.log( 'clicked with this scope: ', this );
  }

  renderCallback() {
    return [
      <div onClick={this.handleClick}>Hello my lord!</div>,
    ];
  }
};

define(App);
davidrus commented 7 years ago

Thank you for responce, but I tried it and still the same error. I created codepen for it, so you can test it: http://codepen.io/davidrus/pen/vxXobW

Hotell commented 7 years ago

Hey folks! so it turns out problem is somewhere between skate and web-components polyfill.

With vanilla js decko works without problems

Demo: http://codepen.io/Hotell/pen/bqwXmm?editors=1010

Hotell commented 7 years ago

Gif or didn't happen right? :D decko-bind-ie11

Hotell commented 7 years ago

all right so after further investigation the issue is definitely with web component polyfills + skate. cc @treshugart @azakus @justinfagnani

It can be fixed though on decko's side.

What's causing the trouble is getter in which you are overriding descriptor ( ie calls it until it explodes )

image

what can be done:

check myBind function ( that's deckos bind redefined )

and ofc gif: decko-bind-ie11 wc-fix

developit commented 7 years ago

Hmm - that breaks caching though - the closure in bind() is scoped to the class definition, whereas get() is scoped to the instance.

I wonder though, why this is only being triggered from skate - maybe this method is getting into an infinite loop because of the reassignment?

treshugart commented 7 years ago

@Hotell I don't see how this is a problem with Skate.

To get around this, you could use http://babeljs.io/docs/plugins/transform-class-properties/ and just do:

handleClick = () => {
  // this is now what you expect without explicit binding
}

This has, at least, seemed to work in IE for me, but maybe that's changed with the polyfill?

developit commented 7 years ago

@treshugart right, just we're talking about @bind as an alternative means of doing that. Internally it's just this:

function bind(target, key, desc) {
  let fn = desc.value;
  return {
    configurable: true,
    get() {
      // only bind to each instance on first access, then cache be replacing property descriptor
      let bound = fn.bind(this);
      Object.defineProperty(this, key, {
        value: bound
      });
      return bound;
    }
  };
}

class Foo extends Component {
  @bind
  handleClick(e) {
    this instanceof Foo;   // true
  }
  render() {
    return <div onClick={this.handleClick} />
                          // ^ binds here only on first render
  }
}

I checked and it seems like MDN actually recommends assigning a property instead of redefining it. Perhaps decko could switch to doing that.

Hotell commented 7 years ago

@treshugart

@Hotell I don't see how this is a problem with Skate.

right sorry, wrong wording ( I didn't meant it's explicitly Skates fault ), but it's a possible culprit

To get around this, you could use http://babeljs.io/docs/plugins/transform-class-properties/ and just do...

right it has one problem though:

if somebody does this ( I know within solid team with solid style guides this wont happen ever :) )

class Foo {

 greeting = this.sayWat()
 sayWat = () => { return 'WAT' }
}

@developit

I checked and it seems like MDN actually recommends assigning a property instead of redefining it. Perhaps decko could switch to doing that.

Hmm interesting, didn't know about this "best practice" I personaly try to avoid delete but looks like it could be useful in here

developit commented 7 years ago

I'm going to try out the simpler overwrite approach from that MDN page and we can check if that fixes the repro :)

finico commented 6 years ago

@developit the solution can be found here https://github.com/andreypopp/autobind-decorator/blob/master/src/index.js#L68-L71

developit commented 6 years ago

Nice - will have to add that lock.

cronon commented 6 years ago

@developit please take a look https://github.com/developit/decko/pull/16