Skip to content
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

aws-cdk-events: jsonTemplate versus textTemplate (take 2) #27

Merged
merged 6 commits into from
Jun 5, 2018

Conversation

eladb
Copy link
Contributor

@eladb eladb commented Jun 5, 2018

Input transformers are primarily designed to allow . formatting JSON documents that will be sent to the
target, in which case the template will look like this:

'{ "foo": <bar> }'

The tokens in angle brackets are first substituted and then the string is parsed as JSON.

If users want to send a string for the target input, they will need the template to look like this:

'"This is a <bar> string"'

(note the double quotes).

To facilitate the common use case where inputs templates should resolve to a string, but also support the JSON option, TargetInputTemplate now accepts two mutually exclusive options: jsonTemplate and textTemplate.

jsonTemplate is simply passed as-is.

If textTemplate is used and a value of type string is passed in, it is JSON.stringifyed. If a non-string value is passed in, we assume it includes tokens, and we simply wrap with double quotes using FnConcat.

Elad Ben-Israel added 6 commits June 3, 2018 14:59
Tokens are resolved when used in event rule target
input templates.
Input transformers are primarily designed to allow
formatting JSON documents that will be sent to the
target, in which case the template will look like
this:

    '{ "foo": <bar> }'

The tokens in angle brackets are first substituted
and then the string is parsed as JSON.

If users want to send a string for the target input,
they will need the template to look like this:

    '"This is a <bar> string"'

(note the double quotes).

To facilitate the common use case where inputs 
templates should resolve to a string, but also support
the JSON option, `TargetInputTemplate` now accepts
two mutually exclusive options: `jsonTemplate` and
`textTemplate`.

`jsonTemplate` is simply passed as-is.

If `textTemplate` is used and a value of type string
is passed in, it is `JSON.stringify`ed. If a non-string
value is passed in, we assume it includes tokens, and
we simply wrap with double quotes using `FnConcat`.
* {
* textTemplate: 'Build <buildid> started',
* pathsMap: {
* buildid: '$.detail.id'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Out of scope for this PR, but can we model this as well.

For example, how am I to know that the current event source has a "detail" member which has an "id" member?

Should be not that hard to slap together some classes that produce the same string, but in a way that can be Ctrl-space autocompleted.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The surface area is pretty big given events can potentially have tons of data associated with them and users will potentially want to extract any of it and transform it when they trigger their target. We might be able to provide some helpers with in the source construct (the one that has onXxx methods) which make it easier to work with certain fields of events emitted by the construct.

At any rate, I've opened #38 to track.

}

if (!inputOptions.jsonTemplate && !inputOptions.textTemplate) {
throw new Error('One of "jsonTemplate" or "textTemplate" are required');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This error message could cover both cases. So simplification (warning, nasty code ahead using !!, just because I can):

if (!!inputOptions.jsonTemplate === !!inputOptions.textTemplate) {
    throw new Error('Exactly one of "jsonTemplate" or "textTemplate" is required');
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this really a simplification?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean, meh. Honestly I don't care.

But it is one fewer if and it's exactly as helpful to users.

The implementation is moderately more complex, using the "bang-bang operator" (a legitimate JavaScript hack I believe) and the fact that we're comparing two booleans for equality.

*/
template?: any;
jsonTemplate?: any;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait, confusing. If jsonTemplate is already supposed to be a JSON-ified object, type it as string, not any. This looks like I can pass in a JSON object.

But at the same time, why CAN'T I pass in a JSON object? That is so much more user-friendly than a pre-stringified object.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I see, it's any instead of string because it also needs to be able to accept a Token, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:sad: tokens are evil (unless maybe we can make them behave like strings... #24)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That only helps insofar as that now we maybe could type this field as string and have users call token.toString() to make that work out.

Let me just leave this notion of Value<string> here again... :)

@eladb eladb merged commit 35c594d into master Jun 5, 2018
@eladb eladb deleted the benisrae/events-template-tokens branch June 5, 2018 18:44
@NGL321 NGL321 added the contribution/core This is a PR that came from AWS. label Sep 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution/core This is a PR that came from AWS.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants