We must declare call as a useEffect hook dependency (docs). We also get a lint error on line 31: "This hook does not specify all of its dependencies: callbiomelint/correctness/useExhaustiveDependencies".

call value is re-calculated on each render and thus Object.is(prevCall, call) will always return false. For example, client.createStripeCheckoutSession(req) creates a new object on each render.
Including call as a hook dependency will have an undesired side-effect of useEffect being called on each render.
To avoid it, we can move the call definition outside of the component (it's value is stable anyway).

it's value is stable anyway

correcting myself here: not always stable, see https://github.com/sourcegraph/sourcegraph/pull/62715#discussion_r1603288864

Good catch!

How could I write a unit test or Storybook test to confirm that we only make these API calls only when needed. Which may be more frequently than the component lifetime, e.g. if you navigate to/from the same page, we may want to refresh the data every time the page is visible. (But not on each paint.)

useApiCaller argument doesn't depend either on component's state or props. As this argument should be included as a hook dependency (https://github.com/sourcegraph/sourcegraph/pull/62715#discussion_r1603095355), to avoid unnecessary hook calls, we should probably define argument value outside the component.

Ah, sorry, actually it depends on customerEmail prop and showPromoCodeField value.
In this case, to avoid re-creating req on each render we should consider wrapping it with useMemo().

The current approach work totally fine, I just want to mention the alternative one.

loading, error, data and response are not independent values IMO. E.g., we shouldn't have data and error defined and isLoading set to true at the same time (optimistic UI updates seem to be outside of the scope given the PR description).

The way these values are updated looks to me like a good use case for useReducer hook.
For example, we can maintain state as a single object:

interface State<T> {
    loading: boolean
    error: Error // or whatever type of an error we want
    data: T
    response: unknown // TODO: replace with actual type
}

and 4 actions that may change it (let's say "call_init", "call_error", "call_success"). This article has an example of a similar approach.

Yeah, I find returning loading, data, error super-clunky too.

I wouldn't have a problem with using a different way to represent this. But personally I'd like to err on the side of having a "dumber but easier to understand" abstraction, than something fancier, but more difficult to use correctly.

Why don't we just keep the "loading, data, error" pattern as-is for now. But once we get this in-place, we can look into improving it with something that requires less tedium.

Since once we streamline our ability to "make API calls", we then would want to streamline how we "handle the responses". (e.g. cleaning up how we deal with loading or error states.)

I think what Taras means is not related to what the hook returns (the interface would remain the same), I think his suggestion was related to how the hook could store its state internally. It's no big deal, though, as the hook is currently pretty simple. I agree it might scale better later with useReducer—that said, I don't think I ever actually used useReducer, and we have two uses of it in the monorepo, so useState is usually sufficient.

We should consider returning a cleanup function not to suffer from race conditions (note on ignore variable in the docs).

Done. Definitely not a scenario I had considered, so thanks for pointing that out.

Please double check I added the check right. Essentially I need to gate all interactions with setState() on !ignore?

Ahh, this is neat, TIL, @taras-yemets. I think the fix is correct.

Cleanup function should be returned by the hook setup function.

useEffect(() => {
  let ignore = false  

  ;(() => {
    // fetch data and set state if `ignore` is still `false`, etc.
  })()

  // cleanup
  return () => {
    ignore = true
  }
}, [...dependencies])  

Ah, true, I overlooked that the return was inside the async function. 🙈

Do we plan to use response in React components?

Hopefully not... but there are situations where we may need to know those details, which is why I left it in the response type.

Essentially the issue is when we get a 4xx response. Usually, it will be a 400 and we can maybe just parse the response body as JSON and assume it is something like { "error": "human-friendly description", "details": ... }.

But we need the response to be available for dealing with more obscure situations, like 401/403. e.g. today we get a 401 error if your SAMS credentials have expired. 🤞 we can "deal with that" within the userApiCaller hook. But I cannot say for certain that we would never want to access that data elsewhere.

Doh, good catch. Fixed.

To be able to use context value we need to wrap a component tree with the context provider (docs). I couldn't find this wrapper so I expect useContext calls will error out.

Yeah, I'm surprised that it worked to... but it totally does!

If you omit the <CodyProApiContext.Provider>...</CodyProApiContext.Provider> it will return the "default value" that is provided here.

I'll update the React components to make this explicit. (Since that's clearly better than having an implicit thing... but at least on my machine, it worked without needing to wrap the component tree.)

If it works without a provider and we need only the default context value, maybe we can use them as create a class instance next to its declaration and export it? They are stable anyway.

export const client = new Client()
export const caller = new CodyProApiCaller()

Then, we can just import them into React components. What do you think?

Also, I wonder if we really need classes here 🤔
Do they add any additional value compared to:

export const getSomeCodyProApiCallArgs = (param): Call<SomeType> => {
    // Not stable, will need to memorize in component.
    // Or, if `param` is not reactive, `getSomeCodyProApiCallArgs` can be called outside of component and be stable.
    return {
    // ... some fields
    param
  }
} 

export function callCodyProApi<T, R> = (call: Call<T>): Promise<{ data?: R; response: Response }> {
  // do stuff
}

?

Then, we can just import them into React components. What do you think?

I'm inclined to check what we have now in as-is, but I would be more than happy to refactor it as we find a better or clearer way to represent the data.

e.g. for the GET API calls, we could side-step the memoization requirement by just exporting a constant. (Since the is no request body.)

export const listCurrentTeamMembersCall = { ... }

export const getMemoizedUpdateSubscriptionSettingsCall(req: ReqBody) = {
     return useMemo(() => ..., [req])
}

That might be an improvement? I would want to wire up more API calls to get a better sense for what impact it would have on the ergonomics.

Also, I wonder if we really need classes here 🤔

I'm not wed to using a class, but one thing that we would be more difficult to do, is having some additional piece of data associated with each call. (e.g. wiring in a "request ID" or some custom header.) Having all of the "get Call<T> objects" be routed through the same class instance makes that a lot easier. (But perhaps that's not a requirement we'd ever have.)

This is a non-blocking comment. Feel free to leave as it is.

We need to call useApiClient to be able to use some Client method which result is used as useApiCaller argument. Given that Client is stateless and its methods are pure functions, I wonder what is the advantage of providing Client instance as a context value compared to making its methids static and using them directly, e.g.,

const { loading, error, data } = useApiCaller(Client.createStripeCheckoutSession(req))

I wonder what is the advantage of providing Client instance as a context value compared to making its methods static and using them directly, e.g.

I certainly can't think of any. I like your way better, which also has the nice property of maybe enabling us to cache/memoize the returned Call<T> objects, too!

nit: Resp sounds to me like a shorthand for Response. As we use it to type the response body and later we return data of type Resp, maybe Data or Result will be more descriptive, WDYT?

Good point, these are all confusing.

Personally, I like the type parameter name as Resp (for Response type). But the fact that the object returned has response: Response, and that is referring to an entirely different thing is... a good sign something is wrong.

data: Data and a doc comment clarifying the two seems like a good call.

We want need response text if the status code is not 2xx. Thus we can move reading response text inside the if block.

We are not sure that response text is a valid JSON string. We may need to wrap response text parsing with try/catch block:

if (fetchResponse.status >= 200 && fetchResponse.status <= 299) {
    try {
        // read response as JSON
        const typedResp = await fetchResponse.json() as Resp 
        return {
            data: typedResp,
            response: fetchResponse,
        }
    } catch (e) {
        // handle error
    }
}

Good call on not waiting on reading the response body on a non-2xx response. 👍

We are not sure that response text is a valid JSON string. We may need to wrap response text parsing with try/catch block:

I don't think we should do that, but please double check my logic. (Since I'm not sure if it is a good idea.)

The problem is that in the Caller, it isn't clear what should be done in the case of any sort of error. (e.g. if the response body fails to unmarshall as JSON, or if the server returns a 5xx.)

So without any clear way to handle errors that crop up, the only choice we have is to:

1. Extend the function's return type to include an optional error, e.g.: { response: Response, data?: Data, error?: Error }.
2. Just let it fail, and require the caller to handle the error instead.

To keep things simpler, I went with option #2. So in the React hook we have:

   try {
       const callerResponse = await caller.call(call)
       ...
   } catch (err) {
      setData(undefined)
      setError(err)
      setResponse(undefined)
      setLoading(false)
   }

And I believe that is "better", in that we do return the error value to the caller. But without needing to complicate the function signature of the Caller interface.

e.g. if there is a failure in unmarshalling the JSON, it will be surfaced to the React component and they will need to respond to it. (Showing some error message in the UI.)

Does that make sense? Does it sound reasonable? Or should we just add an error?: Error field to the object returned from call<Data>(call: Call<Data>): Promise<{ data?: Data; response: Response }>?

nit: Call type definition and usage resemble a subset of RequestInit + resource path. I don't know if "Call" is a descriptive enough name. However, I don't have a better alternative.

Interesting, I wasn't aware of RequestInit. Taking a look though, it is definitely a lot fancier than what we need to do. (And so using that type would probably just lead to trouble, since it would look like we support features that we don't, such as specifying custom request headers.)

Also, I agree Call is not a good name. But I too couldn't come up with a better alternative. Some of the ones I rejected were as follows, LMK if any of those stand out as strictly better.

• ApiRequest
• ApiRequestStub
• RequestMetadata
• CallMetadata

... though I don't feel too bad about having Call here, because in-practice I don't expect it to ever really matter. e.g. it is never needed when you use the API client library. It's just the "thunk-like-thing" that is returned, and passed to the hook that actually performs the HTTP request.

I agree that it's not great and that it's good enough. It's a pretty easy rename later, so let's just leave it for now.

I couldn't find CHECKOUT_SESSION_ID definition? Where do we get it from?

Stripe will automatically find/replace that value when the checkout is complete.
https://docs.stripe.com/payments/checkout/custom-success-page#modify-the-success-url

So the flow is:

1. We render the Stripe iframe, with the clientSecret of XXX which tells Stripe what the configuration settings are.
2. When the checkout is complete, Stripe will redirect the user to "returnUrl" but with the actual CHECKOUT_SESSION_ID
3. On the page that receives that page, we then use that session_id URL query parameter, to load the Checkout Session results from Stripe and see if the checkout was complete or not.

That third step isn't in the Sourcegraph.com code yet. But if you look in the SSC UI, that's what the WelcomePageComponent does.

Can there be a case when we have both error and data.clientSecret?

No, or at least we should try hard to make sure that cannot happen.

So maybe a cleaner (?) way to represent this would be something like:

{
    kind: 'http-response-body-as-json' | 'error'
    result: T | Error
}

I think it's good as-is.

My suggestion was to return early as we do for loading, e.g.

if (loading) {
    return <LoadingSpinner />
}

if (error) {
    return (
        <div>
            <H3>Awe snap!</H3>
            <Text>There was an error creating the checkout session: {error.message}</Text>
        </div>
    )
}

if (data?.clientSecret) {
    return (
        <div>
            <EmbeddedCheckoutProvider stripe={stripePromise} options={{ clientSecret: data.clientSecret }}>
                <EmbeddedCheckout />
            </EmbeddedCheckoutProvider>
        </div>
    )
}

return null

But if we are sure that we won't get truthy values for both error and data.clientSecret, it's good as-is.

We use import type to allow compiler safely drop imports and not load these modules at runtime.

Interesting, I wasn't aware of RequestInit. Taking a look though, it is definitely a lot fancier than what we need to do. (And so using that type would probably just lead to trouble, since it would look like we support features that we don't, such as specifying custom request headers.)

Also, I agree Call is not a good name. But I too couldn't come up with a better alternative. Some of the ones I rejected were as follows, LMK if any of those stand out as strictly better.

• ApiRequest
• ApiRequestStub
• RequestMetadata
• CallMetadata

... though I don't feel too bad about having Call here, because in-practice I don't expect it to ever really matter. e.g. it is never needed when you use the API client library. It's just the "thunk-like-thing" that is returned, and passed to the hook that actually performs the HTTP request.

Doh, good catch. Fixed.

Good point, these are all confusing.

Personally, I like the type parameter name as Resp (for Response type). But the fact that the object returned has response: Response, and that is referring to an entirely different thing is... a good sign something is wrong.

data: Data and a doc comment clarifying the two seems like a good call.

Good call on not waiting on reading the response body on a non-2xx response. 👍

We are not sure that response text is a valid JSON string. We may need to wrap response text parsing with try/catch block:

I don't think we should do that, but please double check my logic. (Since I'm not sure if it is a good idea.)

The problem is that in the Caller, it isn't clear what should be done in the case of any sort of error. (e.g. if the response body fails to unmarshall as JSON, or if the server returns a 5xx.)

So without any clear way to handle errors that crop up, the only choice we have is to:

1. Extend the function's return type to include an optional error, e.g.: { response: Response, data?: Data, error?: Error }.
2. Just let it fail, and require the caller to handle the error instead.

To keep things simpler, I went with option #2. So in the React hook we have:

   try {
       const callerResponse = await caller.call(call)
       ...
   } catch (err) {
      setData(undefined)
      setError(err)
      setResponse(undefined)
      setLoading(false)
   }

And I believe that is "better", in that we do return the error value to the caller. But without needing to complicate the function signature of the Caller interface.

e.g. if there is a failure in unmarshalling the JSON, it will be surfaced to the React component and they will need to respond to it. (Showing some error message in the UI.)

Does that make sense? Does it sound reasonable? Or should we just add an error?: Error field to the object returned from call<Data>(call: Call<Data>): Promise<{ data?: Data; response: Response }>?

Yeah, I'm surprised that it worked to... but it totally does!

If you omit the <CodyProApiContext.Provider>...</CodyProApiContext.Provider> it will return the "default value" that is provided here.

I'll update the React components to make this explicit. (Since that's clearly better than having an implicit thing... but at least on my machine, it worked without needing to wrap the component tree.)

Yeah, I find returning loading, data, error super-clunky too.

I wouldn't have a problem with using a different way to represent this. But personally I'd like to err on the side of having a "dumber but easier to understand" abstraction, than something fancier, but more difficult to use correctly.

Why don't we just keep the "loading, data, error" pattern as-is for now. But once we get this in-place, we can look into improving it with something that requires less tedium.

Since once we streamline our ability to "make API calls", we then would want to streamline how we "handle the responses". (e.g. cleaning up how we deal with loading or error states.)

Done. Definitely not a scenario I had considered, so thanks for pointing that out.

Please double check I added the check right. Essentially I need to gate all interactions with setState() on !ignore?

Good catch!

How could I write a unit test or Storybook test to confirm that we only make these API calls only when needed. Which may be more frequently than the component lifetime, e.g. if you navigate to/from the same page, we may want to refresh the data every time the page is visible. (But not on each paint.)

I wonder what is the advantage of providing Client instance as a context value compared to making its methods static and using them directly, e.g.

I certainly can't think of any. I like your way better, which also has the nice property of maybe enabling us to cache/memoize the returned Call<T> objects, too!

No, or at least we should try hard to make sure that cannot happen.

So maybe a cleaner (?) way to represent this would be something like:

{
    kind: 'http-response-body-as-json' | 'error'
    result: T | Error
}

You can definitely type these more strongly. This works:

Done.

Thanks for creating all these types, @chrsmith. What's your plan to ensure that any engineer touching the back end will sync these types?

Great question. I think this is a situation where we will "add process as needed", rather than trying to formalize something up-front.

It shouldn't be too difficult to remember "if you add APIs to the backend, you probably want to add them to the frontend too". Since ultimately, we won't be able to use those backend APIs unless we export them to the frontend.

So I don't believe this will be a major problem. But if we do find ourselves unintentionally making breaking changes, or not doing a good job keeping these in-sync, we will need to add some sort of unit/integration tests to ensure that we are intentional and know what we are doing when touching these files. (But to start with, let us try erring on the side of trust. #WCGW.)

To further avoid confusion

(I made the mistake of correcting if to is earlier because I and my spellchecker both thought this was a typo.)

Done.

interstitial? (sorry, non-native English speaker here, just the word "intersitular" doesn't seem to exist, so guessing.)

It might not be a real world :P To break it down, it's "inter-site-ular", but the concept is just where you have a "transition" page of some sort.

e.g. the thing some websites show when linking to some unknown 3rd party like:

I reworded the comment to just say this more plainly:

        // BUG: Due to the race conditions between Stripe, the SSC backend,
        // and Sourcegraph.com, immediately loading the Dashboard page isn't
        // going to show the right data reliably. We will need to instead show
        // some prompt, to give the backends an opportunity to sync.