Allow async transforms #753
Conversation
There was a problem hiding this comment.
Also need to update the documentation so the user knows the transformer can be async
src/runtime/step_runner.js
Outdated
| @@ -8,7 +8,7 @@ const {beginTiming, endTiming} = Time | |||
|
|
|||
| async function run({attachmentManager, defaultTimeout, scenarioResult, step, stepDefinition, transformLookup, world}) { | |||
| beginTiming() | |||
| const parameters = stepDefinition.getInvocationParameters({scenarioResult, step, transformLookup}) | |||
| const parameters = await Promise.all(stepDefinition.getInvocationParameters({scenarioResult, step, transformLookup})) | |||
There was a problem hiding this comment.
Why is Promise.all needed? Does getInvocationParameters return a promise or a value? I'd prefer getInvocationParameters always returns a promise.
There was a problem hiding this comment.
getInvocationParameters returns an array where each element is returned from one of the registered Transform#transform functions, which is userland code. If a user wants to write a transform that returns a Promise, Cucumber should be able to deal with that. I've given an example in cucumber/common#108.
Promise.all takes an array of Promise objetcs and returns a new Promise that resolves to a new array of resolved values. If one of the passed in Promise objetcs is not a Promise, then it's wrapped in a Promise#resolve, so both async and regular transform functions will work.
There was a problem hiding this comment.
Alright sounds good. We need to test that if the async transformer rejects that the step gets reported as a failure. At the moment, I believe an async transformer rejecting might just kill the suite.
There was a problem hiding this comment.
Ah yes - hadn't thought of that! Want me to add a test for that or would you prefer to do that yourself?
There was a problem hiding this comment.
We need to test that if the async transformer rejects that the step gets reported as a failure. At the moment, I believe an async transformer rejecting might just kill the suite.
Actually, throwing an exception in a synchronous transform function will also kill Cucumber. See #765.
|
@aslakhellesoy I updated this with the sync / async tests. I think this should be combined with #764 and also with a resolution for #765. The tests fail here because the |
aslakhellesoy
force-pushed
the
async-transforms
branch
from
a5f01fe to
96e6e25
Compare
aslakhellesoy
changed the base branch from
master
to
catch-parameter-transform-errors
|
I've rebased and pushed. Also changed the PR base to |
aslakhellesoy
force-pushed
the
catch-parameter-transform-errors
branch
from
a071aab to
26169b5
Compare
aslakhellesoy
force-pushed
the
async-transforms
branch
from
96e6e25 to
bc0aa10
Compare
|
Rebased again and updated the docs. This should be ready to merge. |
docs/support_files/api_reference.md
Outdated
| @@ -14,6 +14,7 @@ Add a new transform to convert a capture group into something else. | |||
| * `typeName`: string used to refer to this type in cucumber expressions | |||
| * `transformer`: An optional function which transforms the captured argument from a string into what is passed to the step definition. | |||
| If no transform function is specified, the captured argument is left as a string. | |||
| * The function can be synchronous, or it can return a `Promise` of the transformed value. In other words, the function may be `async`. | |||
There was a problem hiding this comment.
I'd suggest this being:
If the function returns a
Promise, the resolved value will be passed to the step definition.
I don't think the async mention is unneeded.
There was a problem hiding this comment.
I don't think it hurts either
There was a problem hiding this comment.
Whatever, change it if you must.
There was a problem hiding this comment.
I understand the ES6/7 reference and would be fine leaving it in if we are more explicit that that is what its referring to. I started JS programming using the async library before switching to promises and think as is, it could be confusing
charlierudolph
changed the base branch from
catch-parameter-transform-errors
to
master
# Conflicts: # docs/support_files/api_reference.md # src/runtime/step_runner.js
|
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
This pull request solves cucumber/common#108