gadget-inc / ggt

The command-line interface for Gadget
https://docs.gadget.dev/reference/ggt
MIT License
11 stars 1 forks source link

Fix log line assertion error #1703

Closed scott-rc closed 1 month ago

scott-rc commented 1 month ago

When we format the value of a log field, we split the value by new line, early return if the number of lines is 0, and assert the first line isn't null to make typescript happy:

const formatValue = (value: string, color: (s: string) => string, indent: number): string => {
  if (!value) {
    return EMPTY;
  }

  const lines = value.split(NEW_LINE);
  if (lines.length === 0) {
    return EMPTY;
  }

  const buf: string[] = [];
  const firstLine = lines.shift();
  assert(firstLine); // can't be null/undefined because we early return if lines.length === 0 above, but typescript doesn't know that so we assert here
  buf.push(color(firstLine));

  // ...
}

The issue with this is that assert(firstLine) will throw if firstLine is an empty string because an empty string is a falsy value 🤦‍♂️. This can be reproduced by logging a field with only a new line, e.g.

import { TestGlobalActionContext } from "gadget-server";

/**
 * @param { TestGlobalActionContext } context
 */
export async function run({ params, logger, api, connections }) {
  logger.info({ foo: "\n" }, "test");
};

Causes this:

  const lines = value.split(NEW_LINE); // ['', '']
  if (lines.length === 0) {
    return EMPTY;
  }

  const buf: string[] = [];
  const firstLine = lines.shift(); // ''
  assert(firstLine); // '' == true
An error occurred while communicating with Gadget

'' == true

If you think this is a bug, click here to create an issue on GitHub.

This PR fixes that by asserting that the firstLine is not nil, which is what I was originally trying to assert:

assert(!isNil(firstLine), "first line is nil");

The reason this ends up as a ClientError is because any errors thrown in the onResponse callback are passed to the onError callback which wraps all errors in a ClientError. We might want to change that...

/cc @pistachiobaby

changeset-bot[bot] commented 1 month ago

🦋 Changeset detected

Latest commit: 740dc0ad5e642279140bd6964906b0f012e13726

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package | Name | Type | | ---- | ----- | | ggt | Patch |

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR