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

Regenerator relies on Function#name, breaks minifiers #283

Closed
monsanto opened this issue Dec 12, 2014 · 10 comments
Closed

Regenerator relies on Function#name, breaks minifiers #283

monsanto opened this issue Dec 12, 2014 · 10 comments
Labels
outdated A closed issue/PR that is archived due to age. Recommended to make a new issue

Comments

@monsanto
Copy link
Contributor

See here.

The author of this code tries to be clever and set GF.name in the line above, but this actually isn't possible to do (try it in Chrome or Firefox, it will just ignore the assignment).

Having "GeneratorFunction" as the constructor name is mandated by the specification. However, I would argue it's not a very important part of the specification, and that convenient minification is much more important. At the very least, the throw line should be removed, so people who don't care what the constructor name is aren't affected. It's worth noting that Regenerator itself does not care what the constructor name is.

However, there is a case for removing the "GeneratorFunction" trick altogether: it would allow for removing the mark() call in the following, improving the readability of the generated code:

root@smogon-dev:~$ echo 'function* foo() { }' | 6to5
"use strict";

var foo = regeneratorRuntime.mark(function foo() {
  return regeneratorRuntime.wrap(function foo$(_context) {
    while (true) switch (_context.prev = _context.next) {
      case 0:
      case "end": return _context.stop();
    }
  }, foo, this);
});

Here's the issue which lead to mark being added: facebook/regenerator#48. The primary reason seems to be allowing the co library to perform some magic to distinguish between regular functions and generator functions. I guess I'm not swayed by this because co isn't very useful when you have async functions (like we do).

@monsanto
Copy link
Contributor Author

Also see mishoo/UglifyJS#552

@sebmck
Copy link
Contributor

sebmck commented Dec 12, 2014

I've done a possible fix. I'm going to spend some time rewriting the regenerator runtime and simplifying it. It'll have to be released with 2.0.0 though due to breaking changes and incompatible API.

@jlongster
Copy link
Contributor

@sebmck I'm glad you fixed it, but how are you going to stay up-to-date with regenerator's runtime? It sounds like you want to completely fork it. Would be nice to see if we could merge changes upstream.

@sebmck
Copy link
Contributor

sebmck commented Dec 13, 2014

@jlongster We wouldn't. Maintenance of the regenerator runtime is minimal compared to everything else.

@sebmck
Copy link
Contributor

sebmck commented Dec 13, 2014

Thanks! Released as of 1.15.0.

@benjamn
Copy link
Contributor

benjamn commented Dec 17, 2014

Please don't spend any time rewriting the regenerator runtime without looping me in!

I'm sincerely confused why no one CC'd me here. You didn't even want to know why that assertion exists? Come on, people: facebook/regenerator#156

@jamiebuilds
Copy link
Contributor

@benjamn I'm sorry you weren't CC'd. 6to5 moves at a really fast pace than other tools so this kind of thing becomes somewhat necessary. However, it's a goal to bring bug fixes and features back into the original libraries.

@monsanto
Copy link
Contributor Author

@benjamn We know why the assertion exists, in fact I discuss it a bit in the first post. I personally don't care if the constructor is the correct name and don't want an error raised if it isn't. I feel easy minification is much, much more important than spec compliance in this corner case.

Note that if you DO care what the constructor name is, you can configure your preferred minifier to not mangle that name. We can even put a note in the documentation.

@aaronshaf
Copy link

In 6to5 2.13.3 I am still getting GeneratorFunction renamed? when using alongside uglifyjs.

@sebmck
Copy link
Contributor

sebmck commented Jan 18, 2015

@aaronshaf Where are you getting the polyfill/runtime from? regenerator/runtime.js doesn't include the GeneratorFunction renamed? error anymore.

ramasilveyra pushed a commit to ramasilveyra/babel that referenced this issue Sep 30, 2017
* account for web.iterable

* extra test, remove unncessary warning
existentialism pushed a commit that referenced this issue Oct 5, 2017
* account for web.iterable

* extra test, remove unncessary warning
JacopKane pushed a commit to JacopKane/babel that referenced this issue Jan 11, 2018
* Update CHANGELOG.md

* Update CHANGELOG.md [skip ci]
@lock lock bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Jan 18, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Jan 18, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated A closed issue/PR that is archived due to age. Recommended to make a new issue
Projects
None yet
Development

No branches or pull requests

6 participants