Skip to content

Spike/rydership markd#1

Open
ideaoforder wants to merge 12 commits into
developfrom
spike/rydership-markd
Open

Spike/rydership markd#1
ideaoforder wants to merge 12 commits into
developfrom
spike/rydership-markd

Conversation

@ideaoforder
Copy link
Copy Markdown

@ideaoforder ideaoforder commented Dec 16, 2025

What is being changed/added/removed in this PR?

This is the initial PR for our fork of onX MCP. It lays down a lot of foundation:

  • Rydership Adapter
    -- getOrders
    -- healthCheck
    -- updateOrder
    -- tests for the above, and then some
  • CI runs via GHA

Checklist

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

Gotchas

  • Requires environmental changes (put details below)
  • Requires an API change (put details below)
  • Includes a database or data migration
  • This PR introduce a breaking / non-backward-compatible change

References

AB#

Dependencies

Requires PR #


Testing Plan

Notes for Reviewer

Other information

@ideaoforder ideaoforder marked this pull request as ready for review December 19, 2025 16:16
Copy link
Copy Markdown

@toddjudd toddjudd left a comment

Choose a reason for hiding this comment

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

Most of these are personal opinions. I'd also recommend setting up a prettier config. we have it all of our integrations and it keeps the repo from having random files with tabs vs spaces, or with unsupported line endings etc. I like having it as a check for every PR.

Everything looks good though, this is a pretty cool project.

@@ -0,0 +1,45 @@
export default {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I'd highly recommend vitest over jest especially for a typescript project. You'll fight with jest and ts-jest where vitest will work out of the box.

});
}

async initialize?(config: AdapterConfig): Promise<void> {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This may be a personal opinion, but I never type function return types unless absolutely needed and I know what I'm doing. Here's a Video with some more info, but the jist is return types can lie. I've not had an issue relying on inference yet.

async createSalesOrder(input: CreateSalesOrderInput): Promise<OrderResult> {
try {
const payload = this.transformCreateSalesOrderInput(input);
const response = await this.client.post<RydershipOrder>('/orders', payload);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This typing is hopeful, line 171 confirms the type which is good. In out integrations we're using zod which let's use define a schema like this

// define a schema
export const RyderOrderSchema = z.object({
  id: z.number(),
})

// convert it to a type when needed
export type RyderOrder = z.infer<typeof RyderOrderSchema>

// use it to parse unknown data into the correct shape
const data = client.get('/orders')
const order = RyderOrderSchema.parse(data) // will throw error
const safeOrder = RyderOrderSchema.safeparse(data) // => { success: true; data: {id:1}}
const safeOrder = RyderOrderSchema.safeparse(data) // => { success: false; errors: ZodError} 

// Then we use them as params on the get call like this: 
async getResource<T extends z.ZodTypeAny>(
    resource: string,
    resourceParser: T,
    id?: number,
    fields?: string[]
  ) {
    const url = id ? `${resource}/${id}` : resource;
    const method = 'get';
    const axiosConfig = {
      url,
      params: {fields},
      method,
    } as AxiosRequestConfig;

    try {
      const {data} = await this._axiosInstance.request<
        z.infer<typeof resourceParser>
      >(axiosConfig);
      return resourceParser.parse(data) as z.infer<typeof resourceParser>;
    } catch (error) {
      throw new ClientError(error, {
        resource: url,
        method,
        client,
      });
    }
  }

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.

2 participants