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

Babel rely on value returned by super() but this is not reliable with Edge #9020

Closed
JSteunou opened this issue Nov 13, 2018 · 11 comments · Fixed by #9060
Closed

Babel rely on value returned by super() but this is not reliable with Edge #9020

JSteunou opened this issue Nov 13, 2018 · 11 comments · Fixed by #9060
Labels

Comments

@JSteunou
Copy link

@JSteunou JSteunou commented Nov 13, 2018

Bug Report

Current Behavior

When using some transpiled code, babel rely on _this from _this = super() but this bug on Edge is still present with latest version microsoft/ChakraCore#4663

Input Code

  • REPL or Repo link if applicable:
export class App extends React.Component {
    state = {
        fruit: 'banana'
    }

    handleClick = (event, ...rest) => {
        this.setState({fruit: 'apple'})
    }

    render() {
        const {fruit} = this.state
        return (
            <h1 onClick={this.handleClick}>I'm a {fruit}</h1>
        )
    }
}

become with preset set on targets: {esmodules: true}

class App extends react__WEBPACK_IMPORTED_MODULE_0___default.a.Component {
  constructor() {
    var _this;

    _this = super(...arguments);
    this.state = {
      fruit: 'banana'
    };

    this.handleClick = function (event) {
      _this.setState({
        fruit: 'apple'
      });
    };
  }

  render() {
    const fruit = this.state.fruit;
    return react__WEBPACK_IMPORTED_MODULE_0___default.a.createElement("h1", {
      onClick: this.handleClick,
      __source: {
        fileName: _jsxFileName,
        lineNumber: 16
      },
      __self: this
    }, "I'm a ", fruit);
  }

}

Expected behavior/code

The initial behavior is that _this should not be undefined but I think that is in the hands of ChakraCore team. Is this something babel can do? There is already this issue which is kinda similar #8019 but also this one which emphasis on the fact that _this should user the value resturned by super() #2279

Environment

  • Babel version(s): v7.1.5
  • Node/npm version: Node 8/npm 6
  • OS: build Ubuntu 18.04, this test on Windows 10 / Edge 17
  • Monorepo no
  • How you are using Babel: loader

Possible Solution

Another possible solution would be to not transpile on rest/spread & default argument function when targets: {esmodules: true} or targets: {browsers: 'Edge >= 14'} because its seems they support it

@babel-bot

This comment has been minimized.

Copy link
Collaborator

@babel-bot babel-bot commented Nov 13, 2018

Hey @JSteunou! We really appreciate you taking the time to report an issue. The collaborators
on this project attempt to help as many people as possible, but we're a limited number of volunteers,
so it's possible this won't be addressed swiftly.

If you need any help, or just have general Babel or JavaScript questions, we have a vibrant Slack
community that typically always has someone willing to help. You can sign-up here
for an invite.

@JSteunou

This comment has been minimized.

Copy link
Author

@JSteunou JSteunou commented Nov 13, 2018

I forgot to say but if the arrow function does not contains rest/spread or default argument value the transpiled code is from

export class App extends React.Component {
    state = {
        fruit: 'banana'
    }

    handleClick = (event) => {
        this.setState({fruit: 'apple'})
    }

    render() {
        const {fruit} = this.state
        return (
            <h1 onClick={this.handleClick}>I'm a {fruit}</h1>
        )
    }
}

to

class App extends react__WEBPACK_IMPORTED_MODULE_0___default.a.Component {
  constructor() {
    super(...arguments);
    this.state = {
      fruit: 'banana'
    };

    this.handleClick = event => {
      this.setState({
        fruit: 'apple'
      });
    };
  }

  render() {
    const fruit = this.state.fruit;
    return react__WEBPACK_IMPORTED_MODULE_0___default.a.createElement("h1", {
      onClick: this.handleClick,
      __source: {
        fileName: _jsxFileName,
        lineNumber: 16
      },
      __self: this
    }, "I'm a ", fruit);
  }

}

hence the emphasis on the possible solution to not transpile rest/spread and default argument for Edge

@JSteunou

This comment has been minimized.

Copy link
Author

@JSteunou JSteunou commented Nov 13, 2018

I tracked down when & why this regression and I found that this fix #8926 introduced it

Rolling back to @babel/preset-env@7.1.0 transpile to

class App extends react__WEBPACK_IMPORTED_MODULE_0___default.a.Component {
  constructor(...args) {
    super(...args);
    this.state = {
      fruit: 'banana'
    };

    this.handleClick = (event, ...rest) => {
      this.setState({
        fruit: 'apple'
      });
    };
  }

  render() {
    const fruit = this.state.fruit;
    return react__WEBPACK_IMPORTED_MODULE_0___default.a.createElement("h1", {
      onClick: this.handleClick,
      __source: {
        fileName: _jsxFileName,
        lineNumber: 16
      },
      __self: this
    }, "I'm a ", fruit);
  }

}

I think the PR #8926 is to aggressive. If I understand well, the need was for object spread with default param, not classical rest/pread nor simple default param.

Update: link correct PR

@frehner

This comment has been minimized.

Copy link

@frehner frehner commented Nov 19, 2018

We just ran into this too, and reverting back to 7.1.0 fixed it. Thank you for your research and workaround, @JSteunou!

@JSteunou

This comment has been minimized.

Copy link
Author

@JSteunou JSteunou commented Nov 20, 2018

no problem, I just hope someone like @nicolo-ribaudo @loganfsmyth or @hzoo will take a look at it, because I think it can be a serious bug, and a very difficult one to analyse to babel users

@nicolo-ribaudo

This comment has been minimized.

Copy link
Member

@nicolo-ribaudo nicolo-ribaudo commented Nov 20, 2018

We should just output super(), _this = this instead of _this = super(). It's only 6 bytes more per class and fixes the issue without introducing any additional flag.
It would probably be an easy PR.

@JSteunou

This comment has been minimized.

Copy link
Author

@JSteunou JSteunou commented Nov 20, 2018

I'm not sure about this, because there was already bugfix around it, and _this = super() was needed. (Cannot find the history, issue around inheritance)

I think the real issue is around this PR #8926 and maybe babel-preset should have a more fine grain around this with Edge.

@joeldenning

This comment has been minimized.

Copy link
Contributor

@joeldenning joeldenning commented Nov 21, 2018

@JSteunou I'm considering submitting a PR for this, but I don't see how #8926 is related? I'm no expert in the babel codebase, but #8926 seems related to arrow functions only, not to classes? Is "destructuring, parameters / defaults, arrow function" somehow related to _this = super()?

@joeldenning

This comment has been minimized.

Copy link
Contributor

@joeldenning joeldenning commented Nov 21, 2018

Oops now I see the connection 👍

@joeldenning

This comment has been minimized.

Copy link
Contributor

@joeldenning joeldenning commented Nov 22, 2018

@JSteunou I just created #9060 which (hopefully) fixes this. This is my first contribution to babel, so I might be unaware of why my solution doesn't work or something, but am hoping it's alright :)

About what you said earlier regarding _this = super() being necessary, do you know which issue that was related to? The solution I implemented was the one that @nicolo-ribaudo suggested in this comment

@JSteunou

This comment has been minimized.

Copy link
Author

@JSteunou JSteunou commented Nov 22, 2018

I'm sorry I cannot get my hand on this old issue :( I was counting on @loganfsmyth memory

Maybe by searching on some file history or tests

nicolo-ribaudo added a commit that referenced this issue Dec 4, 2018
* Not depending on return value of super(). Fixes #9020.

* Feedback from nicolo-ribaudo

* Feedback -- fixing bad call to replaceWithMultiple
@lock lock bot added the outdated label Mar 5, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Mar 5, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
5 participants
You can’t perform that action at this time.