feat Support seam components by azat-co · Pull Request #176 · seamapi/javascript-legacy

azat-co · 2023-04-07T17:18:01Z

No description provided.

https://www.notion.so/Azat-Review-c1afff0644a648a4864f85de3b11c14f

codetheweb · 2023-04-07T18:19:22Z


 export type Event = Flatten<SeamEvent["event_type"]>
+
+export interface ClientSessionI {


🐧 we don't generally add I/T suffixes in this repo

It's a good practice to distinguish interface from a type.

If we are going to introduce this, I think we should rename all interfaces to be consistent then.

Most importantly: we should match existing repo convention to avoid bikeshedding (same rules as open source), so shouldn't be there

RE: general rule

should be prefix rather than suffix to match other Seam repos that use this convention

not sure there's a meaningful enough difference between the interface and a type, the convention makes the most sense where classes are used. Since we almost never use classes (this SDK is a bit of an exception), it's usually unnecessary. Would be strongly opposed to adding a T prefix to types and to a lesser extent the I prefix, since it's typically already distinguished by PascalCase and there are no classes so that's enough

If this comes up more often, having linter is better

@codetheweb @razor-x @seveibar done ✅ 8cddcd7

codetheweb · 2023-04-07T18:20:11Z

-        Authorization: `Bearer ${apiKey}`,
-        ["User-Agent"]: `Javascript SDK v${version} (https://github.com/seamapi/javascript)`,
+        Authorization: `Bearer ${apiKey || clientAccessToken}`,
+        // ["User-Agent"]: `Javascript SDK v${version} (https://github.com/seamapi/javascript)`,


Suggested change

// ["User-Agent"]: `Javascript SDK v${version} (https://github.com/seamapi/javascript)`,

["User-Agent"]: `Javascript SDK v${version} (https://github.com/seamapi/javascript)`,

@codetheweb this line is causing browser errors

@codetheweb are you sure you want it back?

What are the errors? I'm assuming the browser doesn't like when JavaScript tries to override the user agent header. If so we probably need to selectively disable this in the browser environment.

yeah we use the user agent in general to track SDK usage

maybe we could do a typeof window === "undefined" check?

the error is very annoying

Can we use another header name that's not violating some web standard like X-Seam-SDK-Version?

maybe we could

keep user-agent for now until tooling is updated

disable user-agent when in a browser

always send a second header like x-seam-sdk-version with the same info

@codetheweb okay, so I left it for Node but removed for window for backwards compatibility
f55f201 ✅

with seam-sdk-version we have an error I'll open a new issue

Access to XMLHttpRequest at 'http://localhost:3020/devices/list' from origin 'http://localhost:5173' has been blocked by CORS policy: Request header field seam-sdk-version is not allowed by Access-Control-Allow-Headers in preflight response.

codetheweb · 2023-04-07T18:21:18Z

+        },
+      }
+    }
+  }


I think we generally throw instead of directly returning the error

@codetheweb done ✅ df3898e

azat-co · 2023-04-07T21:25:26Z

@seveibar any feedback would be appreciated.

seveibar · 2023-04-08T00:23:24Z

+  }
+}
+
+type catParams = {


PascalCase types, need linter here

Suggested change

type catParams = {

type CATParams = {

done ✅ a4124ff

seveibar · 2023-04-08T00:23:53Z

+    params["user_identifier_key"] = ops.userIdentifierKey
+    try {
+      const response = await axios.post(
+        ops.endpoint + "internal/client_access_tokens/create",


Suggested change

ops.endpoint + "internal/client_access_tokens/create",

ops.endpoint + "internal/client_access_tokens/get_or_create_empty",

it's more clear that it will reuse an existing client session this way

we had this thread in the connect repo

seveibar · 2023-04-08T00:34:55Z

I think the overall approach is good!

seamapi/javascript#176 (comment)

razor-x · 2023-04-14T15:36:39Z

    "device"
  ],
-  "version": "5.12.0",
+  "version": "5.12.1",


How did this version change in this PR branch? This doesn't seems right.

@razor-x the PR bumps the version npm version patch

We should revert this. It will confuse semantic release. Versions are automatically released off of main when PRs are merged

@razor-x d050ec6

razor-x

Some questions on the option passing

razor-x · 2023-04-14T19:14:23Z

  /* Seam API Key */
  apiKey?: string
+  /* Seam Client Access Token */
+  clientAccessToken?: string


I noticed in the new seam provider, we just pass all the keys as apiKey even if it's a clientAccessToken.

Do we still want a new option? Or just have apiKey, let the user pass whatever token they have, and then use the prefix if we need to do special logic?

i think we should be generous in acceptance (since it's detectable) but we shouldn't conflate the options

@razor-x @seveibar done ✅ cfb2be2 and febbd86b46d36cf6f44d70e456b62ba0c15ec2f2

razor-x · 2023-04-14T19:26:28Z

+}
+
+type CSTParams = {
+  publishedKey?: string


Needs updates to match

Suggested change

publishedKey?: string

publishableKey?: string

@razor-x done ✅ seamapi/react@ce81deb and d9a2278

razor-x · 2023-04-14T19:35:04Z

@azat-co To make it easier to grok so we can discuss, here are my proposed interface changes illustrated visually.

seveibar · 2023-04-14T19:51:41Z

@razor-x disagree with making apiKey represent resources (sort of a polymorphic opaque type) if that's the proposal, simplifies implementation slightly at the risk of confusion

seveibar · 2023-04-14T19:48:27Z

 export type Event = Flatten<SeamEvent["event_type"]>
+
+export interface ClientSession {
+  client_access_token_id: string


/client access token/client session token/g

@seveibar yes, and also removed unnecessary properties a7730aa

seveibar · 2023-04-14T19:49:16Z

  events: Event[]
 }
+
+export interface ClientSessionResponseInterface {


Is there a reason for the Interface suffix? Not our usual convention

@seveibar changed ✅ ff9216f

seveibar · 2023-04-14T19:53:26Z

  /* Seam API Key */
  apiKey?: string
+  /* Seam Client Access Token */
+  clientAccessToken?: string


i think we should be generous in acceptance (since it's detectable) but we shouldn't conflate the options

razor-x · 2023-04-14T20:21:01Z

i think we should be generous in acceptance (since it's detectable) but we shouldn't conflate the options

@seveibar

Yes so I think we should either avoid a new option and only have apiKey, or add a new option but not conflate them. So in that case if they try to provide both maybe that is an error, or if they pass in something with a mismatched prefix we should warn them.

seveibar · 2023-04-14T20:36:14Z

Yes so I think we should either avoid a new option and only have apiKey, or add a new option but not conflate them. So in that case if they try to provide both maybe that is an error, or if they pass in something with a mismatched prefix we should warn them.

@razor-x yep agree, a warning/error that they need to use a different parameter is good. Could create an issue from this comment?

This reverts commit 5ab0dea.

…nflate the options be generous in acceptance (since it's detectable) but we shouldn't conflate the options

Co-authored-by: Severin Ibarluzea <seveibar@gmail.com>

seamapi/javascript#176 (comment)

azat-co · 2023-04-17T14:16:12Z

@seveibar

i think we should be generous in acceptance (since it's detectable) but we shouldn't conflate the options

done ✅ cfb2be2

a warning/error that they need to use a different parameter is good.

done ✅ 0e60d22

razor-x · 2023-04-17T15:27:01Z

For reference, this is the branch with the current alpha version https://github.com/seamapi/javascript/tree/ver/5.15.0-alpha

razor-x · 2023-04-17T16:49:30Z

+      clientAccessToken,
+      clientSessionToken,
+    } = getSeamClientOptionsWithDefaults(apiKeyOrOptions)
+    let bearer = `Bearer `


This large nested if/else block is hard to parse. Perhaps easier to maintain if we move this to a function, then we can do early return. 🐧 E.g.,

const getBearerToken({ clientSessionToken, apiKey }) => { if (apiKey || clientAccessToken) { throw new Error( "You can't use clientSessionToken AND specify apiKey or clientAccessToken." ) } if (clientSessionToken} { if (!clientSessionToken?.startsWith("seam_cst")) throw return clientSessionToken }

agree that this function is a bit hard to parse and early return would work better (especially because this bearer var is being mutated)

razor-x · 2023-04-17T16:50:49Z

+      const response: AxiosResponse & {
+        data: { client_session: ClientSession }
+      } = await axios.post(
+        (ops.endpoint ?? "https://connect.getseam.com") +


Can we re-use getSeamClientOptionsWithDefaults here? We are missing the fallback to SEAM_API_URL.

razor-x · 2023-04-17T16:51:46Z

+      const response: AxiosResponse & {
+        data: { client_session: ClientSession }
+      } = await axios.post(
+        (ops.endpoint ?? "https://connect.getseam.com") +


Can we normalize endpoint so we always strip any trailing slashes?

razor-x · 2023-04-17T16:55:57Z

+      // backend mode
+      headers["seam-api-key"] = ops.apiKey
+    }
+    headers["seam-user-identifier-key"] = ops.userIdentifierKey


Suggested change

headers["seam-user-identifier-key"] = ops.userIdentifierKey

if (ops.userIdentifierKey) {

headers["seam-user-identifier-key"] = ops.userIdentifierKey

}

I can't remember exactly but without the conditional this was either generating a type error or a runtime error upstream when userIdentifierKey was null or undefined.

done f5d6f95

seveibar · 2023-04-17T17:43:22Z

  /* Seam API Key */
  apiKey?: string
+  /* Seam Client Access Token */
+  clientAccessToken?: string


clientAccessToken is mislabeled, seam_at should never be used on a client. Maybe personalAccessToken or just accessToken?

(the only thing that should be used on clients are seam_cst)

@seveibar It (AT) can be used on the backend which is the same code, right? We supported AT before this PR.

I think this is just the old name right? Should be clientSessionToken?

seveibar

agree w/ @razor-x 's comments, also would rename clientAccessToken to make sure nobody accidentally uses it on a client.

azat-co · 2023-04-19T04:47:53Z

@seveibar @razor-x yes good catch
remove clientAccessToken ✅ 225366e
userIdentifierKey ✅ f5d6f95
getSeamClientOptionsWithDefaults ✅ 9aacc9b
getBearerToken ✅ 2b0b067

razor-x

Looks good, just noted the one issue with not passing though the axiosOptions.

razor-x · 2023-04-19T05:49:38Z

+      throw new Error("userIdentifierKey is required")
+    }
+    try {
+      const response: AxiosResponse & {


@azat-co I didn't save my comment 😭

Anyway, was just going to say we should make sure to respect axiosOptions, so following the constructor, something like would work

const client = axios.create({ ...axiosOptions, baseURL: endpoint, headers, }) await client.post(...)

Looking closer, I just realized the normal client behavior does a bit more, like always throw SeamAPIError. We should factor out the makeRequest method and use it both places. I will make an issue for that.

seamapi/javascript#184

@razor-x so do you mean instead of using axios directly, create a new instance with options? Makes sense!

Yes, basically we should follow the same logic as the constructor for how we use axios. I created a new issue that better explains the desired behavior. It's ok if we resolve that issue in a follow-up PR.

@razor-x done ✅ 6da07a1

seambot · 2023-04-19T20:47:06Z

🎉 This PR is included in version 5.15.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

azat-co added 7 commits March 31, 2023 21:46

add method for seam components

dc3fc12

Update client.ts

7cd10a7

Update client.ts

84ba5e5

Update client.ts

482765d

Update client.ts

fab51ab

rename everything

5fb84d0

https://www.notion.so/Azat-Review-c1afff0644a648a4864f85de3b11c14f

Update client.ts

cf9c607

azat-co requested a review from codetheweb as a code owner April 7, 2023 17:18

codetheweb reviewed Apr 7, 2023

View reviewed changes

seveibar reviewed Apr 8, 2023

View reviewed changes

azat-co added 9 commits April 10, 2023 17:31

Update client.ts

2d16944

Update client.ts

944d610

Update client.ts

57d6542

rename

8cddcd7

seamapi/javascript#176 (comment)

Update client.ts

f55f201

Update client.ts

8fd1faa

Update client.ts

df3898e

Update client.ts

a4124ff

5.12.1

5ab0dea

razor-x reviewed Apr 14, 2023

View reviewed changes

seveibar reviewed Apr 14, 2023

View reviewed changes

azat-co and others added 7 commits April 17, 2023 07:13

renaming

ff9216f

Revert "5.12.1"

d050ec6

This reverts commit 5ab0dea.

be generous in acceptance (since it's detectable) but we shouldn't co…

cfb2be2

…nflate the options be generous in acceptance (since it's detectable) but we shouldn't conflate the options

Update client.ts

d9a2278

clean up

a7730aa

Update src/seam-connect/client.ts

e949111

Co-authored-by: Severin Ibarluzea <seveibar@gmail.com>

a warning/error that they need to use a different parameter

0e60d22

seamapi/javascript#176 (comment)

azat-co added 2 commits April 17, 2023 08:24

Update client.ts

650842a

Update client.ts

cc3b516

razor-x reviewed Apr 17, 2023

View reviewed changes

seveibar reviewed Apr 17, 2023

View reviewed changes

seveibar approved these changes Apr 17, 2023

View reviewed changes

azat-co added 4 commits April 18, 2023 22:18

Update client.ts

225366e

Update client.ts

f5d6f95

Update client.ts

9aacc9b

refactor to use getBearerToken

2b0b067

razor-x approved these changes Apr 19, 2023

View reviewed changes

razor-x reviewed Apr 19, 2023

View reviewed changes

razor-x mentioned this pull request Apr 19, 2023

Ensure client in static methods has the same behavior as makeRequest #184

Closed

azat-co added 2 commits April 19, 2023 10:51

Merge branch 'main' into support-seam-components

e31aa25

use options

6da07a1

azat-co force-pushed the support-seam-components branch from fd6ca02 to 6da07a1 Compare April 19, 2023 18:22

Merge branch 'main' into support-seam-components

b10b052

azat-co merged commit 58d39bc into main Apr 19, 2023

azat-co changed the title ~~Support seam components~~ feat Support seam components Apr 19, 2023

seambot added the released label Apr 19, 2023


		export type Event = Flatten<SeamEvent["event_type"]>

		export interface ClientSessionI {

	// ["User-Agent"]: `Javascript SDK v${version} (https://github.com/seamapi/javascript)`,
	["User-Agent"]: `Javascript SDK v${version} (https://github.com/seamapi/javascript)`,

	ops.endpoint + "internal/client_access_tokens/create",
	ops.endpoint + "internal/client_access_tokens/get_or_create_empty",

Conversation

azat-co commented Apr 7, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

seveibar Apr 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

azat-co commented Apr 7, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

seveibar commented Apr 8, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

azat-co Apr 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

razor-x left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

azat-co Apr 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

razor-x Apr 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

razor-x commented Apr 14, 2023

Uh oh!

seveibar commented Apr 14, 2023

seveibar Apr 8, 2023 •

edited

Loading

azat-co Apr 17, 2023 •

edited

Loading

azat-co Apr 17, 2023 •

edited

Loading

razor-x Apr 14, 2023 •

edited

Loading