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

Invalid 'this' binding with async arrow function class properties #391

Closed
Jessidhia opened this issue Oct 5, 2016 · 30 comments
Closed

Invalid 'this' binding with async arrow function class properties #391

Jessidhia opened this issue Oct 5, 2016 · 30 comments
Labels

Comments

@Jessidhia
Copy link

Async arrow functions use an invalid 'this' binding when they appear as class properties, as long as the react-hot-loader/babel plugin is present at all.

Arrow functions transformed by react-hot-loader also seem to not respect the spec setting given to the transform-es2015-arrow-functions plugin; however setting spec to false does not help avoid the invalid code either.

I've added some snippets of the problematic code, and also some comments annotating both the input and the output.

class Scroller { // not a React component
  /* ... */
  loadMore = async () => {
    const { data } = await api.getAsync(this.nextUrl, null)
    this.nextUrl = data.next_url
    this.appendData(data)
  }

  // written to verify if it only affected async
  loadMorePromise = () => {
    api.getAsync(this.nextUrl, null)
    .then(({ data }) => {
      this.nextUrl = data.next_url
      this.appendData(data)
    })
  }
}

Snippets of the generated code; in the constructor:

    this.loadMorePromise = function () {
      // _newArrowCheck and .bind(this) added by the spec: true option
      _newArrowCheck(this, _this);

      return this.__loadMorePromise__REACT_HOT_LOADER__.apply(this, arguments);
    }.bind(this);

    this.loadMore = function () {
      _newArrowCheck(this, _this);

      var _ref2 = _asyncToGenerator(regeneratorRuntime.mark(function _callee2() {
        var _args2 = _arguments;
        return regeneratorRuntime.wrap(function _callee2$(_context2) {
          while (1) {
            switch (_context2.prev = _context2.next) {
              case 0:
                _context2.next = 2;
                return _this.__loadMore__REACT_HOT_LOADER__.apply(_this, _args2);

              case 2:
                return _context2.abrupt('return', _context2.sent);

              case 3:
              case 'end':
                return _context2.stop();
            }
          }
        }, _callee2, _this);
      }));

      // wrong Function.name; this one is probably a babel bug
      return function NAME(_x2) {
        return _ref2.apply(this, arguments);
      };
    }.bind(this)();

In the _createClass helper's argument list:

  {
    key: '__loadMorePromise__REACT_HOT_LOADER__',
    value: function __loadMorePromise__REACT_HOT_LOADER__() {
      var _this2 = this;

      // why is this arrow function not using .bind and _newArrowCheck?
      api.getAsync(this.nextUrl, null).then(function (_ref6) {
        var data = _ref6.data;

        _this2.nextUrl = data.next_url;
        _this2.appendData(data);
      });
    }
  }, {
    key: '__loadMore__REACT_HOT_LOADER__',
    value: function () {
      var _ref7 = _asyncToGenerator(regeneratorRuntime.mark(function _callee5() {
        var _ref8, data;

        return regeneratorRuntime.wrap(function _callee5$(_context5) {
          while (1) {
            switch (_context5.prev = _context5.next) {
              case 0:
                _context5.next = 2;
                return api.getAsync(_this5.nextUrl, null);

              case 2:
                _ref8 = _context5.sent;
                data = _ref8.data;

                // _this5 was not defined anywhere, runtime crash
                _this5.nextUrl = data.next_url;
                _this5.appendData(data);

              case 6:
              case 'end':
                return _context5.stop();
            }
          }
        }, _callee5, this);
      }));

      function __loadMore__REACT_HOT_LOADER__() {
        return _ref7.apply(this, arguments);
      }

      return __loadMore__REACT_HOT_LOADER__;
    }()
  }
@danielarias123
Copy link

I am also getting this error, when I remove react-hot-loader, babel compiles as expected

@wuchu
Copy link

wuchu commented Oct 9, 2016

It should be a troubling bug to me, too.

@calesce calesce added the bug label Oct 9, 2016
@calesce calesce added this to the v3.0 milestone Oct 19, 2016
@valerymercury
Copy link

Is it possible to disable hot loader for component/method that fails?

@calesce
Copy link
Collaborator

calesce commented Oct 25, 2016

@valerymercury not currently. it might be worth adding an option to the Babel plugin to opt out of class properties transform.

@calesce
Copy link
Collaborator

calesce commented Oct 31, 2016

@Kovensky: I took a look at this, and I think it's two things: there's a problem with the RHL plugin, where we unnecessarily add async/await to the generated class property. I also think this Babel bug is part of the issue, since we use rest params in the generated code.

@calesce
Copy link
Collaborator

calesce commented Nov 7, 2016

So I was working on a potential fix on calesce/async-fix.

Weirdly enough, if I just run the plugin ahead of time without other Babel transforms and copy the output (using astexplorer), the code runs fine. But if I run it as a normal plugin, it has the broken _this behavior. In both instances I'm using es2015 and stage-2 presets.

Reproduce project here, I'm kind of stuck on this right now. Anyone else have any ideas?

@danielarias123
Copy link

Oddly enough, removing the react-hot-loader/babel plugin from my .babelrc file fixed this problem.

I no longer get the invalid this binding problem on async arrow functions and my react modules hot reload as expected.

@calesce
Copy link
Collaborator

calesce commented Nov 7, 2016

@danielarias123 yeah, but we don't want people to have to remove react-hot-loader/babel because then stateful components don't hot-reload 😄

@danielarias123
Copy link

@calesce hmmm that's weird because both stateless functions that return jsx and stateful React classes are hot reloading for me when the plugin is removed.🤔

@calesce
Copy link
Collaborator

calesce commented Nov 7, 2016

@danielarias123 Right, but they won't retain their state and will remount, you can check by adding componentWillUnmount and see it get called on save.

expbot pushed a commit to expo/xde that referenced this issue Nov 15, 2016
* [xde] Build XDE with Webpack

Build XDE with Webpack.

Benefits:
 - Adds HMR capabilities
 - Tree shaking with web pack 2
 - Potential other optimizations in the future that I haven't thought of yet.

Key points:
 - Keeps node_modules in `app` unbundled -- at the moment, there's no benefit in bundling them.
 - Uses source maps to keep debug-ability  (for some reason source-map support was turned off in Chrome Developer Tools in Electron for me...make sure to turn it on)
 - You can run with hot module reloading turned off or on -- run `npm run start[-local | -staging]-hot` to use it, omit the `-hot` to not.
 - Modified npm scripts so that you don't have to run any watcher script separately. Just run the correct `npm run start-blahblahblha` command and go.
 - No need to `yarn` in both directories -- `yarn` in `dev/xde` will use a postinstall script to `yarn` in the `dev/xde/app` directory and also rebuild any node_modules using electron rebuild.
 - When bundling with Webpack, don't transpile commonJS modules -- Webpack understands these and uses them to do tree shaking.
 - Temporarily, we need to use the `es2015` preset with babel when using HMR -- there is a bug: gaearon/react-hot-loader#391

cc @jesseruder @ide

* [xde] Install React Developer Tools

* [xde] Update yarn.lock.

* [XDE] Fix native dependency installation, update Electron

* [xde] Rebuild when app starts, not on install

* [xde] Fix dirname issue in renderer

* [xde] Clean up gulp + webpack config

* Fix yarn

* [xde] Fixup babel/HMR config

* re yarn

* s/index/renderer

* separate entry point for HMR

fbshipit-source-id: f4e811b
@wesm87
Copy link

wesm87 commented Dec 21, 2016

I've found that the order of the plugins in your .babelrc makes a huge difference. For example, the following produces this error:

plugins: [
  'react-hot-loader/babel',
  'transform-regenerator',
]

This also gives the error:

plugins: [
  'transform-regenerator',
],
env: {
  development: {
    plugins: [
      'react-hot-loader/babel',
    ],
  },
}

However, this works fine:

plugins: [
  'transform-regenerator',
  'react-hot-loader/babel',
]

@wkwiatek
Copy link
Collaborator

wkwiatek commented Jan 9, 2017

I managed to find the root cause of the problem as it appeared for us too.

The problem is strictly connected to the copy of arrow function created as a method in a plugin. However, it's OK when there's no async there.

I assume there's a problem with babel itself rather than methods used there, but there's a part of code I found harmful for this case:

path.insertAfter(newMethod);

I also created an issue with more details on babel's repo: babel/babel#5078

And here is the reproduction repository: https://github.com/wkwiatek/babel-async-test

BTW: @calesce, do you need some help on the v3 milestone?

@mqklin
Copy link

mqklin commented Apr 12, 2017

@wkwiatek is this fixed now? I got this error too. How can I workaround it?

@andtos90
Copy link

@mqklin same here, have you found a workaround?

@LeoLeBras
Copy link

LeoLeBras commented Apr 26, 2017

This doesn't work:

class App extends React.Component {
  myAsyncMethod = async () => {
    return new Promise((resolve) => resolve(true))
  }
  render() {
    return (
      <Button onPress={this.myAsyncMethod} />
    )
  }
}

However, this works fine with:

class App extends React.Component {
  async myAsyncMethod() {
    return new Promise((resolve) => resolve(true))
  }
  render() {
    return (
      <Button onPress={() => this.myAsyncMethod()} />
    )
  }
}

@mqklin
Copy link

mqklin commented Apr 26, 2017

@andreatosatto90 no, I still use v2 :( I tried this #391 (comment) but it causes state reload - #642, so I'm just waiting for updates on this issue.

@Dilatorily
Copy link

I found another workaround:

class App extends React.Component {
  myAsyncMethod = () => async () => {
    return new Promise((resolve) => resolve(true))
  }
  render() {
    return (
      <Button onPress={this.myAsyncMethod()} />
    )
  }
}

@gajus
Copy link

gajus commented Apr 28, 2017

This issue is not limited to async functions. #554

@FourwingsY
Copy link

FourwingsY commented May 2, 2017

I am escaping this issue with:

class App extends React.Component {
  myAsyncMethod = async () => {
    const _ = arguments // eslint-disable-line
    return new Promise((resolve) => resolve(true))
  }
  render() {
    return (
      <Button onPress={this.myAsyncMethod} />
    )
  }
}

or using autobind-decorator

class App extends React.Component {
  @autobind
  async myAsyncMethod() {
    return new Promise((resolve) => resolve(true))
  }
  render() {
    return (
      <Button onPress={this.myAsyncMethod} />
    )
  }
}

@mhagmajer
Copy link

+1

@carlosvini
Copy link

It seems that using the alternative config by removing react-hot-loader/babel from .babelrc and adding react-hot-loader/webpack to webpack.config.js works. It is not the same thing but it is good enough for me.

Source: http://gaearon.github.io/react-hot-loader/getstarted/

Note: react-hot-loader/webpack only works on exported components, whereas react-hot-loader/babel picks up all top-level variables in your files. As a workaround, with Webpack, you can export all the components whose state you want to maintain, even if they’re not imported anywhere else.

@theKashey
Copy link
Collaborator

.. that means that babel transformation, to support arrow functions can be removed!? Let me double check. It should not work.

@ovidiuch
Copy link

FYI (Blindly) downgrading from 3.0.0+ to 3.0.0-beta.7 worked for me.

@GravityMsc
Copy link

some problem when upgrade react-hot-loader, in async arrow function, 'this' is undefined.
I change foo = async () => { } to async foo() { }, and it works.

@theKashey theKashey self-assigned this Nov 22, 2017
@theKashey
Copy link
Collaborator

This is known bug. A solution was found and will come in next version.

@chucksellick
Copy link

FYI (Blindly) downgrading from 3.0.0+ to 3.0.0-beta.7 worked for me.

Can confirm also worked for me.

This issue in v3 (also along with #650) is super frustrating, good to hear next version will be fixing things but right now am stuck on an old version with all warnings suppressed and HMR basically not working properly -- on an otherwise very vanilla create-react-app babel config.

@Jessidhia
Copy link
Author

I found that running a react-hot-loader pass completely separate from a babel pass with all other plugins avoided these issues, even if I edited the react-hot-loader code to make it not skip transforming arrow functions.

The problem seems to happen when it interacts with other transforms; something is executing out of order and changing things before react-hot-loader gets to handle them. At first I thought it was related to mutating nodes and not properly t.cloneing them, but no matter how much cloning was done the result code was still broken.

I then tried printing the node that react-hot-loader was trying to transform to begin with, and it had already been modified by other transforms. Maybe react-hot-loader/babel needs to do all its changes as a sub-traversal on Program enter.

@gregberge
Copy link
Collaborator

This would be definitely fixed in v4 since we do not transpile arrow functions any more.

@mqklin
Copy link

mqklin commented Jan 2, 2018

Confirm, it's fixed in v4

@eugene-beliaev
Copy link

eugene-beliaev commented Feb 15, 2018

I found a workaround that doesn't make you to change any code except the method declaration. Just wrap it in IIFE:

class App extends React.Component {
  myAsyncMethod = (() => async () => {
    return new Promise((resolve) => resolve(true))
  })()

  render() {
    return (
      <Button onPress={this.myAsyncMethod()} />
    )
  }
}

fson pushed a commit to expo/expo-cli that referenced this issue Aug 16, 2018
* [xde] Build XDE with Webpack

Build XDE with Webpack.

Benefits:
 - Adds HMR capabilities
 - Tree shaking with web pack 2
 - Potential other optimizations in the future that I haven't thought of yet.

Key points:
 - Keeps node_modules in `app` unbundled -- at the moment, there's no benefit in bundling them.
 - Uses source maps to keep debug-ability  (for some reason source-map support was turned off in Chrome Developer Tools in Electron for me...make sure to turn it on)
 - You can run with hot module reloading turned off or on -- run `npm run start[-local | -staging]-hot` to use it, omit the `-hot` to not.
 - Modified npm scripts so that you don't have to run any watcher script separately. Just run the correct `npm run start-blahblahblha` command and go.
 - No need to `yarn` in both directories -- `yarn` in `dev/xde` will use a postinstall script to `yarn` in the `dev/xde/app` directory and also rebuild any node_modules using electron rebuild.
 - When bundling with Webpack, don't transpile commonJS modules -- Webpack understands these and uses them to do tree shaking.
 - Temporarily, we need to use the `es2015` preset with babel when using HMR -- there is a bug: gaearon/react-hot-loader#391

cc @jesseruder @ide

* [xde] Install React Developer Tools

* [xde] Update yarn.lock.

* [XDE] Fix native dependency installation, update Electron

* [xde] Rebuild when app starts, not on install

* [xde] Fix dirname issue in renderer

* [xde] Clean up gulp + webpack config

* Fix yarn

* [xde] Fixup babel/HMR config

* re yarn

* s/index/renderer

* separate entry point for HMR
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet