src(templates)!: modify the nodejs event template to accept a cloudevent #356

jcrossley3 · 2021-05-19T21:52:38Z

Pretty-printing the contents of the event and its envelope for each
request. This is handy when invoked as a Knative event sink as some
sources, e.g. Kafka, ignore the response body.

templates/node/events/index.js

lance

Overall lgtm. The overall cleanup is nice, as is the intent. I have a couple of minor wording suggestions. And of course, you know how I feel about the parameter naming. Holding this open for more input on that.

templates/node/events/index.js

lance · 2021-05-20T18:55:20Z

templates/node/events/index.js

-      message: 'No cloud event received'
-    };
-  }
+function handle(context, event) {


I prefer data or eventData as @zroubalik mentioned.

Why?

I know it's just a variable name, but it's obviously important to both of us, so I'm going to reiterate the reasons I gave in slack here:

The name data is not descriptive enough. It's analogous to other poor variable names such as x or object -- it conveys no domain meaning whatsoever.

The name eventData is too descriptive. It risks confusion with the CloudEvent envelope's data field, which is an encoded string. The CloudEvent envelope is an implementation detail that I'd prefer we not expose in the function signature.

The name event is just right. It represents the domain event reconstituted from the CloudEvent envelope according to its data, datacontenttype, and dataschema fields.

The name event is as specific as we can get without knowing which domain is triggering the function. This PR is replacing a function that assumed the domain was user verification. And its second parameter was logically named user. It wasn't called userData because of course it has data. Otherwise, it wouldn't be a parameter at all!

I genuinely cannot fathom why you guys feel so strongly about this. My only guess is that you feel your users will be expecting to work directly with CloudEvent envelopes. And that when they see the parameter name, event, they will expect that it's the envelope. If that's true, then we should be passing the envelope there, not the event reconstituted from its data. Personally, I'd rather have the latter, which is why I stand by the name.

But hey, I like to think I'm reasonable -- sell me!

Don't know what are anybody else expectations, but when I see event I expect it's the whole CloudEvent (including id,source,type etc.).

I am not sure if envelope is good word. Bill once compared CloudEvent to SOAP envelope, but I don't recall the term from CloudEvent spec.

btw data is optional field for CloudEvent it might be nil

Helping as much as I am! :)

I don't think we should necessarily strive for cross-language consistency. We shouldn't impose conventions in one language due to limitations in another. I doubt any nodejs developers will care about the quarkus templates.

Regarding the term "envelope", I heard @nainaz and @slinkydeveloper use it, and I like it as a way to distinguish between a conceptual event and its digital representation. For me, envelope == CloudEvent. For you guys building the stuff, it seems like event == CloudEvent. Not sure about your users (other than myself).

In the world of CloudEvents, event == CloudEvent based on my understanding of this sentence in the spec.

An "event" is a data record expressing an occurrence and its context.

The "context" in this sense is all of the event metadata, e.g. source, type, id and the "occurrence" is the data record itself, i.e. event.data.

Providing event.data as the second parameter was done as a convenience, assuming that the function developer is primarily interested in the data that was generated by the "occurrence". But maybe there is equal interest in the other event properties as well. For example, source and type may be used to filter events or perform conditional actions in the function. If that's the case, I would be in favor of an event parameter that corresponds to the actual CloudEvent.

I'm +1 on the 2nd parameter being an actual CloudEvent if that's the lingua franca of most users. I'm not a "CloudEvent guy" so I apologize for all the noise around this.

I'm assuming for a 2-argument handler, the js-runtime will pass the CE in the second parameter and it will NOT be in the context.
And for a 1-argument handler, only the context will be passed, and it will include the CE as it currently does.

EDIT: ^^ I now realize that the CE needs to remain in the context, due to the way javascript handles arguments

Correct? Want me to create an issue in the js-runtime for that?

Agree with Lance and Matej, if I read event I expect a whole CloudEvent, but I might be biased. And because we are using Knative Eventing, I suppose users (fn developers) will need to know at least some basic stuff about KEventing and it's concepts in order to write the functions properly. So I think that a term CloudEvent would be at least somehow familiar for users, so eventually they might get the same impression as I do. ie. expect a CloudEvent in event parameter.

Not sure if changing the 2nd parameter to be a full CE is somehow useful, I suppose that parameter was introduced so users can easily get the actual data and don't have to parse the CE 🤷‍♂️

When implementing a Handler with Naina (having no familiarity with the function signatures of the language beforehand), we were both thrown off by the second parameter not being a CloudEvent. It seemed an obvious assumption that the event would be the argument. Or, at least this would be a supported default. I would certainly advocate for that change, while at the same time completely agreeing with Jim that we should not force consistency between languages; instead working to their strengths.

lkingland

Looks nice to me! Pending resolution on the CloudEvent parameter and naming of course.

lkingland · 2021-05-24T05:26:02Z

templates/node/events/index.js

  return HTTP.binary(new CloudEvent({
-    source: 'function.verifyUser',
-    type: 'user:verified',
-    data: user
+    source: 'event.handler',
+    type: 'echo',
+    data: event
  }));


So with an eye on our related conversation about accepting an actual CloudEvent named event as the second parameter, it may gain some support from its effect on the default implementation. If we assume that the default handler echoes a cloud event, this cleans up nice:

function handle(context, event) { return event }

That absolutely appeals to my aesthetic!! :)

However, I kinda dig showing a bit of the HTTP library, and I think it's important to set the source & type fields on the returned event.

It might be more useful to the new func user to see data: event.data though, just to reinforce that the event is an actual CloudEvent. But I'd rather not do that until after the runtime is changed to do that. As it stands, this template will work with either the current or the future runtime.

Also, if the event source is a broker, won't returning the event result in an infinite loop?

Also, if the event source is a broker, won't returning the event result in an infinite loop?

That's a good question. I suppose only if the function is subscribed via Trigger, and only filtering on event type - and even then, that assume's the broker is not smart enough to deal with this.

However, I kinda dig showing a bit of the HTTP library, and I think it's important to set the source & type fields on the returned event.

function handle(context, event) { return event.cloneWith({ source: 'echo.fn', type: 'echo' }); }

But I'd rather not do that until after the runtime is changed to do that.

If this lands, nodeshift/faas-js-runtime#96 - will you coordinate with this PR? (TypeScript needs to change as well).

If this lands, boson-project/faas-js-runtime#96 - will you coordinate with this PR? (TypeScript needs to change as well).

Sure, but... define "coordinate" in this context. I think they can each be merged independently. Or I can wait until after #355 lands and adjust those templates, too?

@jcrossley3 I mean, primarily, that this PR is updated to reflect those changes. Due to the fact that these are user facing API changes, we'll also want to update docs/guides/nodejs.md.

Ah, gotcha. I just pushed a commit that updates the nodejs guide.

The result of discussion over here: knative/func#356

lance

This LGTM. Thank you @jcrossley3 for being so patient with this process. In the end I think this is a nice improvement!

lance · 2021-05-25T15:27:44Z

@jcrossley3 I have just realized that the faas-js-runtime dependencies in both event/package.json and http/package.json should be updated to 0.7.1 before this lands.

Pretty-printing the contents of the event and its envelope for each request. This is handy when invoked as a Knative event sink as some sources, e.g. Kafka, ignore the response body. Co-authored-by: Lance Ball <lball@redhat.com>

This will require a change to the faas-js-runtime, but at least the template won't need to change when that's released

jcrossley3 · 2021-05-25T17:23:43Z

@jcrossley3 I have just realized that the faas-js-runtime dependencies in both event/package.json and http/package.json should be updated to 0.7.1 before this lands.

Done

jcrossley3 requested review from lance, lkingland, matejvasek and zroubalik as code owners May 19, 2021 21:52

jcrossley3 force-pushed the nodejs-events branch from 34f34d4 to 0117297 Compare May 20, 2021 12:33

zroubalik reviewed May 20, 2021

View reviewed changes

templates/node/events/index.js Show resolved Hide resolved

lance reviewed May 20, 2021

View reviewed changes

jcrossley3 force-pushed the nodejs-events branch 2 times, most recently from 4621487 to a5b32e7 Compare May 20, 2021 21:42

lkingland reviewed May 24, 2021

View reviewed changes

jcrossley3 added a commit to jcrossley3/faas-js-runtime that referenced this pull request May 24, 2021

Instead of its data, pass the CloudEvent to the handler

9bdae6d

The result of discussion over here: knative/func#356

jcrossley3 mentioned this pull request May 24, 2021

Instead of its data, pass the CloudEvent to the handler nodeshift/faas-js-runtime#96

Merged

lance pushed a commit to nodeshift/faas-js-runtime that referenced this pull request May 24, 2021

Instead of its data, pass the CloudEvent to the handler (#96)

e28d00d

The result of discussion over here: knative/func#356

lance approved these changes May 24, 2021

View reviewed changes

lkingland approved these changes May 25, 2021

View reviewed changes

lance changed the title ~~Simplify the nodejs events template~~ src(templates)!: modify the nodejs event template to accept a cloudevent May 25, 2021

jcrossley3 and others added 3 commits May 25, 2021 12:57

Simplify the nodejs events template

ff76d47

Pretty-printing the contents of the event and its envelope for each request. This is handy when invoked as a Knative event sink as some sources, e.g. Kafka, ignore the response body. Co-authored-by: Lance Ball <lball@redhat.com>

Expect the event parameter to be the entire CloudEvent

fad522d

This will require a change to the faas-js-runtime, but at least the template won't need to change when that's released

Update nodejs guide to reflect 2nd arg's type is CloudEvent

ca2f3ec

jcrossley3 force-pushed the nodejs-events branch from 05e4e8c to ca2f3ec Compare May 25, 2021 16:57

Templates now using faas-js-runtime 0.7.1

34244c5

lance merged commit caf0659 into knative:main May 25, 2021

This was referenced May 25, 2021

chore: release 0.15.0 #346

Merged

chore: release 0.15.0 jrangelramos/func#2

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

src(templates)!: modify the nodejs event template to accept a cloudevent #356

src(templates)!: modify the nodejs event template to accept a cloudevent #356

jcrossley3 commented May 19, 2021

lance left a comment

lance May 20, 2021

jcrossley3 May 20, 2021 •

edited

matejvasek May 20, 2021

matejvasek May 20, 2021

matejvasek May 20, 2021

jcrossley3 May 20, 2021

lance May 20, 2021

jcrossley3 May 20, 2021 •

edited

zroubalik May 21, 2021 •

edited

lkingland May 24, 2021

lkingland left a comment

lkingland May 24, 2021

jcrossley3 May 24, 2021

jcrossley3 May 24, 2021

lance May 24, 2021

lance May 24, 2021

lance May 24, 2021

jcrossley3 May 24, 2021

lance May 24, 2021

jcrossley3 May 24, 2021

lance left a comment

lance commented May 25, 2021

jcrossley3 commented May 25, 2021

src(templates)!: modify the nodejs event template to accept a cloudevent #356

src(templates)!: modify the nodejs event template to accept a cloudevent #356

Conversation

jcrossley3 commented May 19, 2021

lance left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcrossley3 May 20, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcrossley3 May 20, 2021 • edited

Choose a reason for hiding this comment

zroubalik May 21, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lkingland left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lance left a comment

Choose a reason for hiding this comment

lance commented May 25, 2021

jcrossley3 commented May 25, 2021

jcrossley3 May 20, 2021 •

edited

jcrossley3 May 20, 2021 •

edited

zroubalik May 21, 2021 •

edited