Fix for #4943 "Calling an async function with default parameter as function for arguments checking handled synchonous" #5688

hulkish · 2017-05-01T23:30:50Z

Q	A
Patch: Bug Fix?	yes
Major: Breaking Change?	no
Minor: New Feature?	no
Deprecations?	no
Spec Compliancy?	yes
Tests Added/Pass?	yes
Fixed Tickets	Fixes #4943
License	MIT
Doc PR	no
Dependency Changes	no

This change reflects this suggestion: #4943 (comment)

mention-bot · 2017-05-01T23:30:51Z

@hulkish, thanks for your PR! By analyzing the history of the files in this pull request, we identified @loganfsmyth, @spicyj and @hzoo to be potential reviewers.

codecov · 2017-05-01T23:37:12Z

Codecov Report

Merging #5688 into 7.0 will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##              7.0    #5688   +/-   ##
=======================================
  Coverage   84.56%   84.56%           
=======================================
  Files         283      283           
  Lines        9742     9742           
  Branches     2735     2735           
=======================================
  Hits         8238     8238           
- Misses        991      992    +1     
+ Partials      513      512    -1

Impacted Files	Coverage Δ
packages/babel-helpers/src/helpers.js	`100% <ø> (ø)`	⬆️
packages/babel-traverse/src/visitors.js	`85.71% <0%> (-0.96%)`	⬇️
packages/babel-traverse/src/path/context.js	`85.34% <0%> (ø)`	⬆️
...kages/babel-generator/src/generators/statements.js	`97.19% <0%> (+0.56%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 79c1eed...62c0fe7. Read the comment docs.

sophiebits · 2017-05-01T23:57:14Z

This seems simple enough to me. Can you write a test for this?

hulkish · 2017-05-02T00:01:07Z

@spicyj sure, is there an area i should focus on for this test? Like... should I look for if the arguments are being applied outside of the promise block?

sophiebits · 2017-05-02T00:11:03Z

I think testing for the original issue – something that throws immediately upon execution – and verifying the correct behavior at runtime would be the best test here.

hulkish · 2017-05-02T00:16:33Z

@spicyj gotcha, ill work on that now

hulkish · 2017-05-02T01:38:07Z

@spicyj hows that?

sophiebits · 2017-05-02T01:57:50Z

Sorry, I should have been clearer. Can you add an exec.js test as described in https://github.com/babel/babel/blob/7.0/CONTRIBUTING.md#writing-tests that verifies the code actually works at runtime? That is, not just checking the compiled output. That way, if someone changes this in the future, it will be obvious why the function call needs to be inside the new Promise().

Also, not sure it makes sense to have this test in packages/babel-core – any reason you didn't put both in the async-to-generator folder?

hulkish · 2017-05-02T02:00:07Z

@spicyj no reason, ill move it

hulkish · 2017-05-02T03:16:42Z

@spicyj better?

sophiebits

This seems all right although the test case could likely be as simple as:

assert.doesNotThrow(async function() {
throw new Error('should be thrown after a tick');
});

I'll let someone else review too.

hulkish · 2017-05-02T04:37:47Z

@spicyj agree the test can be simpler, but i think its good to check for doesNotThrow & also a check to see that the asynchronously thrown error is caught properly.

loganfsmyth · 2017-05-02T04:54:27Z

packages/babel-helpers/src/helpers.js

@@ -216,8 +216,10 @@ helpers.asyncGeneratorDelegate = template(`
 helpers.asyncToGenerator = template(`
  (function (fn) {
    return function () {
-      var gen = fn.apply(this, arguments);
+      var _args = arguments;
+      var _promiseWrapper = this;
      return new Promise(function (resolve, reject) {


If you want to change this to an arrow function, that's fine with me. Seems like it would be worth it.

loganfsmyth · 2017-05-02T05:07:28Z

packages/babel-plugin-transform-async-to-generator/test/fixtures/regression/4943/exec.js

+  if (err instanceof AssertionError) {
+    throw err;
+  }
+});


I believe the runner expects a promise to be returned if it is going to have async operations that need to be waited for. As it is, this will queue up a bunch of async operations, but not wait for them to complete before moving on to a new test.

I guess the easiest solution might be to wrap this whole test in return (async function(){ ... })();?

hulkish · 2017-05-02T06:17:33Z

@spicyj @loganfsmyth ok:

simplified the tests
made the exec ran test return an async function
improved original change to asyncToGenerator helper template by using an arrow function on returned promise block. This gets transformed to the same behavior:

function _asyncToGenerator(fn) { return function () { var _this = this, _arguments = arguments; return new Promise(function (resolve, reject) { var gen = fn.apply(_this, _arguments); [...]

Also, this part is necessary to avoid the unhandled rejected promise warning:

      .then(() => {
        throw new Error('should not occcur');
      })
      .catch(() => true);

The then part is just a redundancy.

hulkish · 2017-05-02T06:24:36Z

core-js@2.4.1 node_modules/core-js
make[1]: Leaving directory `/home/travis/build/babel/babel'
make test-only
make[1]: Entering directory `/home/travis/build/babel/babel'
./scripts/test.sh
undefined:233
    return new Promise((resolve, reject) => {
                                         ^^
SyntaxError: Unexpected token =>
    at Object.<anonymous> (/home/travis/build/babel/babel/packages/babel-helper-transform-fixture-test-runner/lib/index.js:186:61)

hmm.. im guessing this fails in ci builds (and not locally) because of node version differences.... should i revert my arrow function change back to final output as pasted in my last comment?

This is the line it dies on:

var babelHelpers = eval((0, _babelCore.buildExternalHelpers)(null, "var"));

Could be wrong, but although the asyncToGenerator helper template eventually gets transformed - it isn't transformed for this eval. Therefore I'm wondering if it's unrelated to this issue and just a bug with the test runner.

Maybe Unrelated to this issue, but possible solution:

Might be due to an incomplete lerna bootstrap from any variants of defined recursively-linked dependencies of babel/packages/*.
Maybe change this line:

babel/Makefile

Line 76 in c46ef65

./node_modules/.bin/lerna bootstrap

to:

./node_modules/.bin/lerna bootstrap --hoist

hulkish · 2017-05-02T06:39:13Z

Went ahead and removed the arrow function and just put the expected output in the helper template. That ok?

xtuc · 2017-05-02T10:47:14Z

Thanks for your PR 👍

The PR is against master (Babel 6) which still support Node < 0.12 and doesn't support arrow functions syntax. I would like to port this to Babel 7 using an arrowFunction, ok for you @loganfsmyth ?

existentialism · 2017-05-02T13:01:14Z

Yes, let's target to 7.0

xtuc · 2017-05-02T16:53:37Z

Ok, as said by @loganfsmyth, would be great to change it to an arrowFunction then.

hulkish · 2017-05-02T16:55:35Z

@xtuc do i need to do this? If so, can I get some advice on where to apply the change in order to add support for this per the issue explained here.

hzoo · 2017-05-02T18:08:15Z

Can you change the base of the PR to target 7.0? Then you can use arrow functions fine. Then there won't be a need to apply the es2015 preset either in the exec test.

hulkish · 2017-05-02T19:19:00Z

@hzoo so I made these changes, resolved the conflicts and pushed... but it's not being reflected here for some reason...what do?

hulkish · 2017-05-02T19:27:43Z

Do I need to move my changes to the corresponding 7.0 branch in my forked repo?

hulkish · 2017-05-02T20:39:18Z

ok now im a little confused.... i put the base to 7.0, but then there were conflicts... how do i fix this?

hulkish · 2017-05-02T20:55:42Z

nvm, got it.

xtuc · 2017-05-02T21:09:01Z

@hulkish are you fine with the arrowFunction change? The issue was from Node < 0.12. You need to correct the old tests fixtures which still use the function.

hulkish · 2017-05-02T21:11:06Z

@xtuc yea im good with it.. I just fixed the test for it.

hulkish · 2017-05-02T21:17:18Z

Ok, so back to this on the ci builds, lol:

make[1]: Leaving directory `/home/ubuntu/babel'
make test-only
make[1]: Entering directory `/home/ubuntu/babel'
./scripts/test.sh

undefined:233
    return new Promise((resolve, reject) => {
                                          ^
SyntaxError: Unexpected token >
    at Object.<anonymous> (/home/ubuntu/babel/packages/babel-helper-transform-fixture-test-runner/lib/index.js:186:61)
    at Module._compile (module.js:456:26)
    at Module._extensions..js (module.js:474:10)

…async-default-arguments test

…se base branch

…mrproved change to self/arguments refs by using arrow function on returned promise

hulkish · 2017-05-03T01:17:54Z

hulkish · 2017-05-03T13:23:42Z

@loganfsmyth anything else i need to do here?

existentialism · 2017-05-03T13:47:40Z

packages/babel-plugin-transform-async-to-generator/test/fixtures/regression/4943/options.json

@@ -0,0 +1,7 @@
+{
+  "presets": ["es2015"],


Still needed post move to 7.0?

I didn't realize this should be moved out for 7.0 branch, ill go ahead and do that.

how's that?

loganfsmyth · 2017-05-03T22:21:28Z

The easiest fix for those failures would be to add use strict to those tests, I think.

loganfsmyth · 2017-05-03T22:21:57Z

By the way, thanks so much for this, I know it's been a lot of back and forth, hope it's not too bad of an experience.

hulkish · 2017-05-03T22:57:05Z

@loganfsmyth no no, its cool... its far more important that the outcome is desired than to rush this anyway

hulkish · 2017-05-03T22:57:35Z

@loganfsmyth im kinda stuck now though...travis build fails but circlici build works haha

jridgewell · 2017-05-03T23:20:45Z

Node 4 doesn't support destructuring, you'll have to enable the transform to run the exec test.

hulkish · 2017-05-04T00:15:02Z

@jridgewell @loganfsmyth ok i think this is the one!!

loganfsmyth · 2017-05-04T04:44:51Z

Thanks so much!

sophiebits approved these changes May 2, 2017

View reviewed changes

loganfsmyth reviewed May 2, 2017

View reviewed changes

hulkish changed the base branch from master to 7.0 May 2, 2017 18:11

hulkish added 4 commits May 2, 2017 19:59

updated test fixture to reflect change

c4ec382

corrected the apply to use correct scope and arguments

a72c506

added regression test for issue babel#4943, added async-to-generator/…

d2b3be8

…async-default-arguments test

added regression test for issue babel#4943

39d6e38

hulkish force-pushed the master branch from 1399615 to 05e6a4e Compare May 3, 2017 00:15

hulkish and others added 2 commits May 2, 2017 20:38

switched back to using arrow function, since now pointing to v7 relea…

2af5856

…se base branch

simplified async-to-generator regression test for issue babel#4943, i…

e3b4f69

…mrproved change to self/arguments refs by using arrow function on returned promise

hulkish force-pushed the master branch from 05e6a4e to e3b4f69 Compare May 3, 2017 00:40

updated text fixtures

1fe9667

existentialism reviewed May 3, 2017

View reviewed changes

removed es2015 preset usage from issue babel#4943 regression exec test

f85fa6c

hulkish added 2 commits May 3, 2017 18:22

added use strict to test fixture

5a2c69c

added use strict to test fixture

6e2660e

hulkish added 3 commits May 3, 2017 19:42

added destructing transform to test options

cb4fc20

removed use strict from exec test

04d9c7a

added parameters & destructing transforms to test

62c0fe7

loganfsmyth mentioned this pull request May 4, 2017

Remove babel-plugin-external-helpers in favor of modular helpers #5699

Open

loganfsmyth merged commit d5aa6d3 into babel:7.0 May 4, 2017

hzoo added the PR: Bug Fix 🐛 A type of pull request used for our changelog categories label May 15, 2017

lock bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Oct 6, 2019

lock bot locked as resolved and limited conversation to collaborators Oct 6, 2019

Fix for #4943 "Calling an async function with default parameter as function for arguments checking handled synchonous" #5688

Fix for #4943 "Calling an async function with default parameter as function for arguments checking handled synchonous" #5688

Conversation

hulkish commented May 1, 2017

mention-bot commented May 1, 2017

codecov bot commented May 1, 2017 • edited

Codecov Report

sophiebits commented May 1, 2017

hulkish commented May 2, 2017

sophiebits commented May 2, 2017

hulkish commented May 2, 2017

hulkish commented May 2, 2017

sophiebits commented May 2, 2017

hulkish commented May 2, 2017

hulkish commented May 2, 2017

sophiebits left a comment

Choose a reason for hiding this comment

hulkish commented May 2, 2017 • edited

loganfsmyth May 2, 2017

Choose a reason for hiding this comment

loganfsmyth May 2, 2017

Choose a reason for hiding this comment

hulkish commented May 2, 2017 • edited

hulkish commented May 2, 2017 • edited

hulkish commented May 2, 2017 • edited

xtuc commented May 2, 2017

existentialism commented May 2, 2017

xtuc commented May 2, 2017

hulkish commented May 2, 2017 • edited

hzoo commented May 2, 2017

hulkish commented May 2, 2017

hulkish commented May 2, 2017 • edited

hulkish commented May 2, 2017 • edited

hulkish commented May 2, 2017

xtuc commented May 2, 2017

hulkish commented May 2, 2017

hulkish commented May 2, 2017 • edited

hulkish commented May 3, 2017

hulkish commented May 3, 2017

existentialism May 3, 2017

Choose a reason for hiding this comment

hulkish May 3, 2017

Choose a reason for hiding this comment

hulkish May 3, 2017

Choose a reason for hiding this comment

loganfsmyth commented May 3, 2017

loganfsmyth commented May 3, 2017

hulkish commented May 3, 2017

hulkish commented May 3, 2017

jridgewell commented May 3, 2017

hulkish commented May 4, 2017 • edited

loganfsmyth commented May 4, 2017

codecov bot commented May 1, 2017 •

edited

hulkish commented May 2, 2017 •

edited

hulkish commented May 2, 2017 •

edited

hulkish commented May 2, 2017 •

edited

hulkish commented May 2, 2017 •

edited

hulkish commented May 2, 2017 •

edited

hulkish commented May 2, 2017 •

edited

hulkish commented May 2, 2017 •

edited

hulkish commented May 2, 2017 •

edited

hulkish commented May 4, 2017 •

edited