Contributing to roslibjs: From Defeat to Merged PR

While scrolling through GitHub during my free time, I came across roslibjs the standard Robot Operating System JavaScript library. After researching the project, I learned that roslibjs enables web applications to communicate with robots from within a browser. The library uses WebSockets to connect with rosbridge which provides a JSON API to ROS functionality for non-ROS programs, supporting publishing, subscribing, service calls, and other ROS functionality. The core architecture made me interested to work on this issue. This issue was open since June 12, and from the description, it seemed trivial to fix. However, I didn't account for the architectural complexity involved. After communicating with a maintainer about tackling this issue, I dove in. Here's the PR that I made.

The Biggest Mistake: Being Lost in the Codebase

I started by reading the entire documentation, which provided valuable insights but took me far beyond the scope of what I actually needed. I spent three days trying to understand the complete architecture, which turned out to be overkill. Ultimately, I only needed to check the status field in Action.sendGoal instead of the result field which was a much simpler fix than I had anticipated.

Taking a Break to Reset

After exhausting myself by exploring the wrong parts of the codebase, I took a one-week break from the issue. I needed to step back and recharge. But after that week, I pushed myself to restart with a fresh perspective. Because I thought without even trying and doing my best how can I accept the defeat?
With proper research, I found the Rosbridge Protocol and ROS 2 GoalStatus documentation exactly what I needed to solve the problem.

My Initial Approach

I identified the bug in Action.sendGoal, where the method incorrectly relied on the result field as a success/failure indicator. This caused STATUS_CANCELED and other status codes to be handled incorrectly. I updated the sendGoal method in src/core/Action.ts to check message.status against GoalStatus.STATUS_SUCCEEDED as the primary success indicator, routing all other status codes to the failure callback with descriptive error messages.

My Implementation

// Check status code instead of result field
const status = message.status as GoalStatus;

if (status === GoalStatus.STATUS_SUCCEEDED) {
  resultCallback(message.values as TResult);
} else {
  // Handle failures with appropriate error messages
 // with switch case statements
  failedCallback(errorMessage);
}

Functional Review Feedback

A maintainer provided excellent feedback. While the added functionality looked good, they suggested also checking message.result to catch edge cases where it might be false, potentially indicating backend implementation issues. I revised the code to validate both fields:

        if (status === GoalStatus.STATUS_SUCCEEDED && message.result) {
          resultCallback(message.values);
        }

Also, after doing that I did not need to do casting , because TypeScript already knew the type so I did not need to write as TResult so, I removed it.

TypeScript Review and Future-Proofing

Another maintainer told me that it looks good, but also suggested a "future-proofing nit": instead of writing error messages directly in the function, I should create a custom error class that handles template interpolation. This would provide a proper error type for future use.

My Attempt for Error Class

export class GoalError extends Error {
  constructor(status: GoalStatus | number | string, baseError?: unknown) {
    const base = typeof baseError === "string" ? baseError : "";
    let message: string;

    switch (status) {
      case GoalStatus.STATUS_CANCELED:
        message = `Action was canceled${base ? `: ${base}` : ""}`;
        break;
      case GoalStatus.STATUS_ABORTED:
        message = `Action was aborted${base ? `: ${base}` : ""}`;
        break;
      case GoalStatus.STATUS_CANCELING:
        message = `Action is canceling${base ? `: ${base}` : ""}`;
        break;
      case GoalStatus.STATUS_UNKNOWN:
        message = `Action status unknown${base ? `: ${base}` : ""}`;
        break;
      default:
        message = `Action failed with status ${String(status)}${base ? `: ${base}` : ""}`;
    }

    super(message);
    this.name = "GoalError";
    Object.setPrototypeOf(this, GoalError.prototype);
  }
}

My approach to using this error class was somewhat naive. The maintainer helped refine it, and the PR was eventually merged. I felt proud ,because I went from doing nothing to fixing error-prone code in a real open-source project. Later, I examined the maintainer's implementation to understand the cleaner approach:

Maintainer's implementation

class GoalError extends Error {
  override name = "GoalError";
  constructor(status: GoalStatus, errorValue?: string) {
    super(`${makeErrorMessage(status)}${errorValue ? `: ${errorValue}` : ""}`);
  }
}

function makeErrorMessage(status: GoalStatus) {
  switch (status) {
    case GoalStatus.STATUS_CANCELED:
      return `Action was canceled`;
    case GoalStatus.STATUS_ABORTED:
      return `Action was aborted`;
    case GoalStatus.STATUS_CANCELING:
      return `Action is canceling`;
    case GoalStatus.STATUS_UNKNOWN:
      return `Action status unknown`;
    default:
      return `Action failed with status ${String(status)}`;
  }
}

The maintainer extracted the message generation logic into a separate makeErrorMessage function, making the code more modular and easier to test.The template interpolation cleanly combines the base error message with optional additional context. This demonstrated to me even as programming becomes more abstract, clean code principles stay the same.

Test Cases

In most open-source projects, fixing a bug or implementing a feature requires comprehensive test coverage. I needed to create test cases that strictly matched the real protocol.

interface ActionResultMessageBase {
  op: "action_result";
  id: string;
  action: string;
  status: number;
}

Then I split success and failure into two shapes:

interface FailedActionResultMessage extends ActionResultMessageBase {
  result: false;
  values?: string;
}

interface SuccessfulActionResultMessage extends ActionResultMessageBase {
  result: true;
  values: { result: number };
}

type ActionResultMessage =
  | FailedActionResultMessage
  | SuccessfulActionResultMessage;

This pattern is often described as a discriminated union in TypeScript: a union where one field tells you which shape you have. That lets TypeScript narrow types automatically and keeps your code safer.

I also created feedback messages:

interface ActionFeedbackMessage {
  op: "action_feedback";
  id: string;
  action: string;
  values: { current: number };
}

type ActionMessage = ActionResultMessage | ActionFeedbackMessage;

ActionMessage is then a union of result messages and feedback messages. This reminded me that learning is never wasted. Even though I spent the first week diving too deep into the architecture, that exploration helped me design better test types later on.

Writing a Test Case: STATUS_ABORTED Example

Let me walk through how I created the test case for STATUS_ABORTED to show my thought process:

it("should call failedCallback when action is aborted (STATUS_ABORTED)", () => {
      const resultCallback = vi.fn();
      const failedCallback = vi.fn();

      action.sendGoal(
        { target: 10 },
        resultCallback,
        undefined,
        failedCallback,
      );

      const abortedMessage: FailedActionResultMessage = {
        op: "action_result",
        id: "test-id",
        action: "/test_action",
        values: "Action aborted due to error",
        status: GoalStatus.STATUS_ABORTED,
        result: false,
      };

      messageHandler?.(abortedMessage);

      expect(failedCallback).toHaveBeenCalled();
      expect(resultCallback).not.toHaveBeenCalled();
    });

I created a mock message (abortedMessage) that matches the protocol structure for a failed action. The important parts are: status: GoalStatus.STATUS_ABORTED (tells us it failed) and result: false (confirms the failure). Then I just check that the failure callback gets called and the success callback doesn't. Simple as that.

Conclusion

This contribution taught me a lot more than how to fix a bug. I realized if I put effort, care about the code and read the documentation, I can solve most of the bugs. Also, I realized that Senior developers think long-term. That "future-proofing nit" about a dedicated error class showed me how they design for clarity and future changes.Through this single PR, I learned about protocols, WebSockets, error handling, TypeScript type design (including discriminated unions), and cleaner architecture patterns. The roslibjs maintainers are proactive, kind, open to guiding me through the process, I really appreciate them. Most importantly, I went from feeling stuck and intimidated to shipping a real fix in a production library. That feeling is absolutely worth the initial discomfort.

Comments

[[e-mail]]

NICE JOB!!

[ElshadHu]

Thanks, I appreciate it🙌