-
Notifications
You must be signed in to change notification settings - Fork 4k
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
fix(stepfunctions): allow condition on array #4340
Conversation
A Choice state after a Parallel state receives an array as input.
Thanks so much for taking the time to contribute to the AWS CDK ❤️ We will shortly assign someone to review this pull request and help get it
|
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
@@ -177,8 +177,8 @@ enum CompoundOperator { | |||
class VariableComparison extends Condition { | |||
constructor(private readonly variable: string, private readonly comparisonOperator: ComparisonOperator, private readonly value: any) { | |||
super(); | |||
if (!variable.startsWith('$.')) { | |||
throw new Error(`Variable reference must start with '$.', got '${variable}'`); | |||
if (!variable.startsWith('$.') && !variable.startsWith('$[')) { |
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 was curious about the performance implications of using two startsWith
vs. a single RegExp, and it seems like the latter is 57% faster on Chrome 77.
Not a change request by any means, just sharing 😄
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.
Thanks, even faster when using .test
instead of .match
: https://jsperf.com/str-startswith-vs-regex/2, updated PR
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.
Oh wow, I didn't expect the gain to be this big with .test(
.
I also picked a worst case scenario for startsWith
using $a
as a test.
But anyway, thanks for nitpick-y change 👍
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Thank you for contributing! Your pull request is now being automatically merged. |
A Choice state after a Parallel state receives an array as input.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license