feat: Add waitForActionAttempt option

[razor-x]

Still need to decide on if we will provide this behavior in the client methods. However this method is a prerequisite for anything like that.

[razor-x]

We could introduce an option to SeamHttp called awaitActionAttempts and either codegen something like this

  async lockDoor(
    body?: LocksLockDoorBody,
  ): Promise<LocksLockDoorResponse['action_attempt']> {
    const { data } = await this.client.request<LocksLockDoorResponse>({
      url: '/locks/lock_door',
      method: 'post',
      data: body,
    })
    if (this.awaitActionAttempts) {
      return resolveActionAttempt(data.action_attempt, this)
    }
    return data.action_attempt
  }

Note the client would NEVER wait for action attempts returned by actionAttemps.get.

Or use an axios interceptor

if (options.awaitActionAttempts) this.client.interceptors.response.use(actionAttemptInterceptor)

export const actionAttemptInterceptor = async <T>(response: T): Promise<T> => {
  if (isActionAttemptResponse(response)) await resolveActionAttempt(response.data, this)
  return response
}

This one looks clearer, but has some downsides:

• Have to implement isActionAttemptResponse. Unclear what to do for endpoints with resource and action attempt.
• isActionAttemptResponse must be false for actionAttemps.get since this is used by resolveActionAttempt
• Endpoint dependent logic is hidden in the interceptor, makes debugging much harder.

[razor-x]

Another idea: introduce a special method:

seam.actionAttempts.resolve<T extends ActionAttempt>(params: T | Promise<T> | string): Promise<SuccessfulActionAttempt<T>>

I don't like this as much since it breaks parity with the API. It is kinder for tree-shaking though,

Perhaps better is a top level method, this way the client does not need to be passed into the function

seam.resolveActionAttempt(actionAttempt)

This approach is not tree-shakeable though.

[seveibar]

Other SDKs introduce utility functions at the sdk level in various ways, many do it on the HTTP calls themselves e.g. a parameter wait_for_action_attempt. I think it's okay to have a utility but I don't think we want to have this call signature as the standard call signature for utility functions, what about this to start?

Suggested change

	await resolveActionAttempt(seam.locks.unlockDoor({ device_id }), seam)
	await seam.utils.resolveActionAttempt(seam.locks.unlockDoor({ device_id }))

I'd also recommend renaming to waitForActionAttempt because resolve* is definitely correct in promise terminology but not really how you would describe the behavior in plain english.

Suggested change

	await resolveActionAttempt(seam.locks.unlockDoor({ device_id }), seam)
	await seam.utils.waitForActionAttempt(seam.locks.unlockDoor({ device_id }))

[razor-x]

I actually named it waitForActionAttempt and then renamed it before opening this PR. 😅 We can go back.

Adding wait_for_action_attempt as a param is something we can (and probably should) do: this PR is mainly to introduce the stand-alone method which is useful IMO on its own and a prerequisite for that. Introducing more options needs to be sorted out first in terms of the generator and how it's exposed, e.g., wait_for_action_attempt would only be valid on a subset of endpoints, and how would the non-payload params work together, e.g., how do we expose pagination?

I agree about the call signature. I saw this issue afterwards when I realized it needs a client. Opened the PR to get some ideas before refactoring.

I would like to avoid a generic utils as a pattern: I always feel it's an anti pattern. AWS SDK has an example here I think we can borrow from:

import { DynamoDBClient } from "@aws-sdk/client-dynamodb";
import { DynamoDBDocumentClient, GetCommand } from "@aws-sdk/lib-dynamodb";


const client = new DynamoDBClient({});
const docClient = DynamoDBDocumentClient.from(client);

I believe it belongs in the action attempts namespace, but since we don't want to conflate top level API endpoint methods with library methods, this could be be SeamHttpActionAttemptsLib.fromClient(client), used like

await seam.actionAttempts.lib.waitForActionAttempt(seam.locks.unlockDoor({ device_id }))

[seveibar]

I'm not sure there's a good reason to separate HTTP-mirrored calls from utilities in SDK-first design. Both have network calls behind the scenes, and it's common for SDKs to rewrite the backing http call. I think the HTTP methods inform how to structure the SDK but don't need to be explicitly separated, from a consumers method they don't see the difference. So I'm recommending we remove ".lib." and agree that namespacing is good for these methods but looking at this it seems more clear that this should be a parameter and not a method, it's going to clutter the namespace a bit- can we just add it as a parameter without the utility function?

await seam.actionAttempts.waitForActionAttempt(seam.locks.unlockDoor({ device_id }))

vs

await seam.locks.unlockDoor({ device_id, waitForActionAttempt: true })

[razor-x]

Ok, but I think we should put this in the second argument and not mixup the API request body

[seveibar]

I think it should all be in the same named argument list. Some considerations:

• Axios convention says options can be in the second argument
  • Axios options actually can change the request content, it's not a meaningful distinction and they can also be combined into the same body via some signatures
• Positions don't inherently mean anything, we're saying it can be control flow options if we add a second positional parameter
• Creating a conceptual understanding about what's an HTTP parameter and what's an SDK parameter doesn't help customers AFAIK, it's just an SDK w/ parameters to people who use the SDK
• I think SDK-first design would lump these things in the same named parameter list (some parameters like sync already kind of change control flow)
  • arguably taking SDK-first to the extreme would change some other things, I'm not sure exactly what

[razor-x]

Something else I remember I thought about earlier worth mentioning: some methods may not take json as the first argument (if the req body is binary). So combination will be impossible

[seveibar]

The case mismatch is a pretty good argument, I hadn't noticed that because Seam has made me case-blind, IMO that's a strong enough basis for the javascript SDK to do it as a second argument 👍

[seveibar]

More justification: Basically we've already established we're breaking convention around data structures used by the HTTP API, I hadn't realized that (it isn't true for python where this is a named argument and also the positional approach isn't really possible without sacrificing ergonomics)

[razor-x]

Ok, how does this look (see README)?

[razor-x]

A slight alternative option would be to do waitForActionAttempt: bool | { timeout, pollingInterval }. This way the advanced config is hidden and scoped.

We can add these options to the constructor as well to allow users to set a default.

[codetheweb]

API looks good 👍

would be nice to mark generated files with

// .gitattributes
<glob> linguist-generated

as when I'm reviewing it's not immediately clear which ones were manually edited