-
Notifications
You must be signed in to change notification settings - Fork 3
Conversation
Looks good :) @jfmengels Could you review 0f850a2 ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great. And pretty nice to read too (haven't looked at Babel code in a while).
I just have one concern for which I'd like to see some tests, but otherwise, LGTM :)
if (hoisted) { | ||
path.replaceWithMultiple( | ||
hoisted.expressions.concat( | ||
t.expressionStatement(path.node) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are replacing the Callxpression by an ExpressionStatement here. Would the following test code stll work, or would Babel crash or something because you are building an odd AST?
test('should foo...', async t => {
t.is(
t.throws(foo(await quux())).message,
'Could not find X'
);
});
My worry is that you can't AFAIK pass a statement as the argument of a function (here, the `t.is(){ call). But I don't know how Babel would react to that. Would you mind adding a test case and see how that pans out?
I would be fine with not entirely supporting this edge case, but it should at the very least not crash.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea I doubt that'll work… in your example t.throws
is inside an ArgumentExpression
so we'd have to walk the path back to the test implementation and detect any ArgumentExpression
nodes, then prepend the hosted statements to node's parent.
But then what about:
test(async t => {
(function* () {
const err = await t.throws(foo(yield))
t.true(err.message === 'hahaha')
})()
})
That scenario is further complicated by this plugin running after async/await has been transpiled into a generator…
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, what should we do about it? Just ignore it as an edge-case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't had the time to get back to this. We could explore whether we can run this plugin before any other presets and plugins. Alternatively the generator compilation (for Node.js v4 and up) should reduce to yield
expressions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Relevant: babel/babel#3281
Alternatively the generator compilation (for Node.js v4 and up) should reduce to yield expressions.
I would be ok with only supporting Node.js 4 for this.
The helper wraps the original argument with a new function. This is a problem if the argument contains yield or await expressions, since the function they now run is is not a generator or marked as asynchronous. Instead hoist these expressions so they're resolved outside of the helper. Fixes avajs/ava#987.
f77bbe8
to
c916cff
Compare
Another approach may be to rewrite t.throws(
(
t._throwsArgStart(),
t._throwsArgEnd(fn(await foo))
),
TypeError
)
|
@novemberborn I like that approach. I can't really think of any immediate downsides of doing that. @jfmengels Thoughts? ⬆️ |
Closing in favor of #8. |
General code cleanup and dependency upgrades, as well as a fix for avajs/ava#987 (see last commit).
The
await
andyield
expressions may still throw of course. I'm wondering if we want to capture those exceptions with this helper? They wouldn't be due to misuse oft.throws()
however.