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

Not-so-pure IIFE async generators stripped out in production. #6973

Closed
phyllisstein opened this Issue Dec 4, 2017 · 12 comments

Comments

Projects
None yet
6 participants
@phyllisstein

phyllisstein commented Dec 4, 2017

Choose one: is this a bug report or feature request?

Bug report.

Input Code

;(async () => {
  await import('app')
})()

Babel/Babylon Configuration (.babelrc, package.json, cli command)

module.exports = {
  presets: [
    ['@babel/env', {
      loose: true,
      modules: process.env.npm_lifecycle_event === 'build:es' ? false : 'commonjs',
      useBuiltIns: 'usage',
    }],
    '@babel/typescript',
    '@babel/react',
    '@babel/stage-0',
  ],
}

Expected Behavior

Babel should transpile the async IIFE down such that the app module is imported, in both development and production environments.

Current Behavior

Following #6803, Babel adds a /*#__PURE__*/ annotation to the output code:

/*#__PURE__*/
_asyncToGenerator(
/*#__PURE__*/
regeneratorRuntime.mark(function _callee() {
  return regeneratorRuntime.wrap(function _callee$(_context) {
    while (1) {
      switch (_context.prev = _context.next) {
        case 0:
          _context.next = 2;
          return import("./src/app");

        case 2:
        case "end":
          return _context.stop();
      }
    }
  }, _callee, this);
}))();

In production builds, UglifyJS drops the function by default, as described in its documentation:

  • side_effects (default: true) -- Pass false to disable potentially dropping
    functions marked as "pure". A function call is marked as "pure" if a comment
    annotation /*@__PURE__*/ or /*#__PURE__*/ immediately precedes the call. For
    example: /*@__PURE__*/foo();

Pasting the output code into https://skalman.github.io/UglifyJS-online/ results in its being uglified entirely away. I'm not sure whether this was the behavior intended by the original pull request, but the heuristic that determines a function's purity seems to paint with a pretty broad brush:

https://github.com/satya164/babel/blob/42c740ca0a009ac4019a339c7f9be9293c93ab8c/packages/babel-helper-remap-async-to-generator/src/index.js#L76-L85

Possible Solution

Roll back #6803.

Context

We use async IIFEs to coordinate module loading and webfont availability. It was surprising when we found that the application wouldn't boot at all in production, and took an afternoon's digging to understand that the "pure" annotation was the culprit.

Your Environment

software version(s)
Babel v7.0.0-beta.34
node v9.2.0
npm v5.6.0
Operating System macOS 10.13.2 (17C83a)
@babel-bot

This comment has been minimized.

Show comment
Hide comment
@babel-bot

babel-bot Dec 4, 2017

Collaborator

Hey @phyllisstein! 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.

Collaborator

babel-bot commented Dec 4, 2017

Hey @phyllisstein! 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.

@nicolo-ribaudo

This comment has been minimized.

Show comment
Hide comment
@nicolo-ribaudo

nicolo-ribaudo Dec 4, 2017

Member

Babel should output something like

(
  /*#__PURE__*/
  _asyncToGenerator(
    /*#__PURE__*/
    regeneratorRuntime.mark(function _callee() {
      return regeneratorRuntime.wrap(function _callee$(_context) {
        while (1) {
          switch (_context.prev = _context.next) {
            case 0:
              _context.next = 2;
              return import("./src/app");
    
            case 2:
            case "end":
              return _context.stop();
          }
        }
      }, _callee, this);
    }))
)();

So the __PURE__ annotation is attatched to the correct function call.

Member

nicolo-ribaudo commented Dec 4, 2017

Babel should output something like

(
  /*#__PURE__*/
  _asyncToGenerator(
    /*#__PURE__*/
    regeneratorRuntime.mark(function _callee() {
      return regeneratorRuntime.wrap(function _callee$(_context) {
        while (1) {
          switch (_context.prev = _context.next) {
            case 0:
              _context.next = 2;
              return import("./src/app");
    
            case 2:
            case "end":
              return _context.stop();
          }
        }
      }, _callee, this);
    }))
)();

So the __PURE__ annotation is attatched to the correct function call.

@Andarist

This comment has been minimized.

Show comment
Hide comment
@Andarist

Andarist Dec 4, 2017

Member

The full rollback of #6803 is not needed. The mentioned heuristics need to be fine-tuned and exclude cases when async function is immediately invoked. This is somewhat caused by the fact that it takes only a single annotation to mark all nested calls as pure. Examples:

/*#__PURE__*/fn1(fn2())
/*#__PURE__*/fn()()()()

I've once raised concern about this behaviour here but we couldn't think of any real life scenarios when that would be dangerous and we might have found the first one here, cc @kzc.

Anyway - we need to be more careful about inserting those annotations, but the same kinda applies to any transformation which babel does. Only the real world and real applications can verify assumptions made by the plugins and if the logic behind them is bullet proof.

Member

Andarist commented Dec 4, 2017

The full rollback of #6803 is not needed. The mentioned heuristics need to be fine-tuned and exclude cases when async function is immediately invoked. This is somewhat caused by the fact that it takes only a single annotation to mark all nested calls as pure. Examples:

/*#__PURE__*/fn1(fn2())
/*#__PURE__*/fn()()()()

I've once raised concern about this behaviour here but we couldn't think of any real life scenarios when that would be dangerous and we might have found the first one here, cc @kzc.

Anyway - we need to be more careful about inserting those annotations, but the same kinda applies to any transformation which babel does. Only the real world and real applications can verify assumptions made by the plugins and if the logic behind them is bullet proof.

@nicolo-ribaudo

This comment has been minimized.

Show comment
Hide comment
@nicolo-ribaudo

nicolo-ribaudo Dec 4, 2017

Member

I just realized that what I said in the comment above doesn't make sense, since even if the call is pure it can't be dropped (otherwise it becomes undefined())

Member

nicolo-ribaudo commented Dec 4, 2017

I just realized that what I said in the comment above doesn't make sense, since even if the call is pure it can't be dropped (otherwise it becomes undefined())

@kzc

This comment has been minimized.

Show comment
Hide comment
@kzc

kzc Dec 4, 2017

@Andarist Uglify's behavior regarding this pure annotation is set in stone at this point - it is used in the wild and is not likely going to change. I'd advise to accommodate it with parentheses or other means for this use case. I don't think that it's a [good first issue] since #6803 is breaking code. Either a revert or a timely PR is in order by someone familiar with the issue and the Babel code base.

kzc commented Dec 4, 2017

@Andarist Uglify's behavior regarding this pure annotation is set in stone at this point - it is used in the wild and is not likely going to change. I'd advise to accommodate it with parentheses or other means for this use case. I don't think that it's a [good first issue] since #6803 is breaking code. Either a revert or a timely PR is in order by someone familiar with the issue and the Babel code base.

@Kovensky

This comment has been minimized.

Show comment
Hide comment
@Kovensky

Kovensky Dec 5, 2017

Member

I'm not sure it's that big of a deal to change Uglify's behavior. It would be a non-breaking change -- a code size regression, maybe, but it would, in no way, cause working code to not work anymore. But you may have different guarantees in your project.

Member

Kovensky commented Dec 5, 2017

I'm not sure it's that big of a deal to change Uglify's behavior. It would be a non-breaking change -- a code size regression, maybe, but it would, in no way, cause working code to not work anymore. But you may have different guarantees in your project.

@kzc

This comment has been minimized.

Show comment
Hide comment
@kzc

kzc Dec 5, 2017

Uglify is behaving as intended in the top post. The call expression is greedy.

kzc commented Dec 5, 2017

Uglify is behaving as intended in the top post. The call expression is greedy.

@kzc

This comment has been minimized.

Show comment
Hide comment
@kzc

kzc Dec 5, 2017

@Andarist Is this the fix you had in mind?

--- a/packages/babel-helper-remap-async-to-generator/src/index.js
+++ b/packages/babel-helper-remap-async-to-generator/src/index.js
@@ -65,23 +65,25 @@ const awaitVisitor = {
 export default function(path: NodePath, file: Object, helpers: Object) {
   path.traverse(awaitVisitor, {
     file,
     wrapAwait: helpers.wrapAwait,
   });
 
+  const isCall = path.parentPath.isCallExpression();
+
   path.node.async = false;
   path.node.generator = true;
 
   wrapFunction(path, helpers.wrapAsync);
 
   const isProperty =
     path.isObjectMethod() ||
     path.isClassMethod() ||
     path.parentPath.isObjectProperty() ||
     path.parentPath.isClassProperty();
 
-  if (!isProperty) {
+  if (!isProperty && !isCall) {
     annotateAsPure(
       path.isDeclaration() ? path.get("declarations.0.init") : path,
     );
   }
 }

Note: untested.

kzc commented Dec 5, 2017

@Andarist Is this the fix you had in mind?

--- a/packages/babel-helper-remap-async-to-generator/src/index.js
+++ b/packages/babel-helper-remap-async-to-generator/src/index.js
@@ -65,23 +65,25 @@ const awaitVisitor = {
 export default function(path: NodePath, file: Object, helpers: Object) {
   path.traverse(awaitVisitor, {
     file,
     wrapAwait: helpers.wrapAwait,
   });
 
+  const isCall = path.parentPath.isCallExpression();
+
   path.node.async = false;
   path.node.generator = true;
 
   wrapFunction(path, helpers.wrapAsync);
 
   const isProperty =
     path.isObjectMethod() ||
     path.isClassMethod() ||
     path.parentPath.isObjectProperty() ||
     path.parentPath.isClassProperty();
 
-  if (!isProperty) {
+  if (!isProperty && !isCall) {
     annotateAsPure(
       path.isDeclaration() ? path.get("declarations.0.init") : path,
     );
   }
 }

Note: untested.

@Andarist

This comment has been minimized.

Show comment
Hide comment
@Andarist

Andarist Dec 5, 2017

Member

@kzc Yeah pretty much, that's why I've marked it as "good first issue". Babel@7 is at the moment at the beta stage and I believe that premature reverts would only stall things, it is expected that some bugs might still appear during that stage - we obviously hope to fix all of them as soon as possible.

As beta versions should be pinned in your package.json, I believe more appropriate course of action is to downgrade your babel until a fix is released.

Member

Andarist commented Dec 5, 2017

@kzc Yeah pretty much, that's why I've marked it as "good first issue". Babel@7 is at the moment at the beta stage and I believe that premature reverts would only stall things, it is expected that some bugs might still appear during that stage - we obviously hope to fix all of them as soon as possible.

As beta versions should be pinned in your package.json, I believe more appropriate course of action is to downgrade your babel until a fix is released.

@Kovensky

This comment has been minimized.

Show comment
Hide comment
@Kovensky

Kovensky Dec 8, 2017

Member

Ended up claiming this because it kinda affected me at work 😅

Couldn't update to newer betas to include other bug fixes because I'd be bringing in this bug instead.

Member

Kovensky commented Dec 8, 2017

Ended up claiming this because it kinda affected me at work 😅

Couldn't update to newer betas to include other bug fixes because I'd be bringing in this bug instead.

@nicolo-ribaudo

This comment has been minimized.

Show comment
Hide comment
@nicolo-ribaudo

nicolo-ribaudo Dec 9, 2017

Member

Fixed by #6999

Member

nicolo-ribaudo commented Dec 9, 2017

Fixed by #6999

@phyllisstein

This comment has been minimized.

Show comment
Hide comment
@phyllisstein

phyllisstein Dec 9, 2017

Thanks for picking this up @Kovensky , and thanks for the heads-up @nicolo-ribaudo ! Super grateful for your time and your help.

phyllisstein commented Dec 9, 2017

Thanks for picking this up @Kovensky , and thanks for the heads-up @nicolo-ribaudo ! Super grateful for your time and your help.

@lock lock bot added the outdated label May 3, 2018

@lock lock bot locked as resolved and limited conversation to collaborators May 3, 2018

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