Skip to content

Conversation

grant
Copy link
Member

@grant grant commented Sep 28, 2021

Add better native logging to the CloudEvent class.

Proposed Changes

Logging a CloudEvent should call the toString of the CloudEvent.

Fixes #432

Signed-off-by: Grant Timmerman <timmerman+devrel@google.com>
Signed-off-by: Grant Timmerman <timmerman+devrel@google.com>
Signed-off-by: Grant Timmerman <timmerman+devrel@google.com>
@grant grant self-assigned this Sep 28, 2021
@grant grant requested a review from lance September 28, 2021 01:37
Signed-off-by: Grant Timmerman <timmerman+devrel@google.com>
Copy link
Member

@lance lance left a comment

Choose a reason for hiding this comment

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

Thanks for making this change, @grant - just a small request re: the test

* The native `console.log` value of the CloudEvent.
* @return The string representation of the CloudEvent.
*/
[Symbol.for("nodejs.util.inspect.custom")]() {
Copy link
Member

Choose a reason for hiding this comment

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

This is a pretty clever approach. :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Co-authored-by: Lance Ball <lball@redhat.com>
@grant
Copy link
Member Author

grant commented Sep 29, 2021

I'm getting errors with the index type of the symbol. Want to go with the existing test and perhaps you can add changes if appropriate after?

Signed-off-by: Grant Timmerman <timmerman+devrel@google.com>
@lance
Copy link
Member

lance commented Sep 30, 2021

I'm getting errors with the index type of the symbol. Want to go with the existing test and perhaps you can add changes if appropriate after?

I was able to get the test to pass like this:

  it("serializes as JSON with raw log", () => {
    const ce = new CloudEvent({ ...fixture, data: { lunch: "tacos" } });
    const sym = (Symbol.for("nodejs.util.inspect.custom") as unknown) as string;
    const f = (ce[sym] as CallableFunction).bind(ce);
    expect(ce.toString()).to.deep.equal(f());
  });

Also, to eliminate the warnings that are being generated, the CE function should look like this (note {string} in the @return jsdoc annotation, and the : string return annotation on the function declaration):

  /**
   * The native `console.log` value of the CloudEvent.
   * @return {string} The string representation of the CloudEvent.
   */
  [Symbol.for("nodejs.util.inspect.custom")](): string {
    return this.toString();
  }

Signed-off-by: Grant Timmerman <timmerman+devrel@google.com>
@grant grant requested a review from lance September 30, 2021 20:34
@grant
Copy link
Member Author

grant commented Sep 30, 2021

Great, thanks @lance. PTAL.

Copy link
Member

@lance lance left a comment

Choose a reason for hiding this comment

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

LGTM

@grant grant merged commit b38a48f into cloudevents:main Oct 4, 2021
@grant grant deleted the grant_native_log branch October 4, 2021 18:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CloudEvent omits data from console output
3 participants