Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

init(ts/hooks/useApiCall): add hook for consuming api promises #2533

Merged
merged 3 commits into from
Apr 16, 2024
Merged

init(ts/hooks/useApiCall): add hook for consuming api promises #2533

merged 3 commits into from
Apr 16, 2024

Conversation

firestack
Copy link
Member

@firestack firestack commented Apr 5, 2024 •
edited
Loading

This adds a generic hook for consuming Promise's, we currently only have a need for this with API results.

This will be combined with #2531 to make implementing API endpoints which may return an error easier.

You can see this code in use on the PR #2534

@firestack firestack self-assigned this Apr 5, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented Apr 5, 2024

Coverage of commit d1f33c4

Summary coverage rate:
  lines......: 94.3% (3222 of 3418 lines)
  functions..: 73.9% (1334 of 1805 functions)
  branches...: no data found

Files changed coverage rate: n/a

Download coverage report

@firestack firestack marked this pull request as ready for review April 5, 2024 15:09
@firestack firestack requested a review from a team as a code owner April 5, 2024 15:09
@firestack firestack force-pushed the kf/jj/znvltrqptqmy branch from d1f33c4 to 2442cf5 Compare April 5, 2024 17:37
@github-actions GitHub Actions
Copy link

github-actions bot commented Apr 5, 2024

Coverage of commit 7a1b995

Summary coverage rate:
  lines......: 94.3% (3222 of 3418 lines)
  functions..: 73.9% (1334 of 1805 functions)
  branches...: no data found

Files changed coverage rate: n/a

Download coverage report

@github-actions GitHub Actions
Copy link

github-actions bot commented Apr 5, 2024

Coverage of commit 7a1b995

Summary coverage rate:
  lines......: 94.3% (3222 of 3418 lines)
  functions..: 73.9% (1334 of 1805 functions)
  branches...: no data found

Files changed coverage rate: n/a

Download coverage report

@firestack firestack force-pushed the kf/jj/znvltrqptqmy branch from 7a1b995 to e7c4c6d Compare April 5, 2024 18:32
@github-actions GitHub Actions
Copy link

github-actions bot commented Apr 5, 2024

Coverage of commit e7c4c6d

Summary coverage rate:
  lines......: 94.3% (3222 of 3418 lines)
  functions..: 73.9% (1334 of 1805 functions)
  branches...: no data found

Files changed coverage rate: n/a

Download coverage report

@firestack firestack force-pushed the kf/jj/znvltrqptqmy branch from e7c4c6d to c4f8bf8 Compare April 5, 2024 20:18
@github-actions GitHub Actions
Copy link

github-actions bot commented Apr 5, 2024

Coverage of commit c4f8bf8

Summary coverage rate:
  lines......: 94.3% (3222 of 3418 lines)
  functions..: 73.9% (1334 of 1805 functions)
  branches...: no data found

Files changed coverage rate: n/a

Download coverage report

@firestack firestack marked this pull request as draft April 9, 2024 17:02
@firestack firestack marked this pull request as ready for review April 11, 2024 14:21
joshlarson
joshlarson reviewed Apr 11, 2024
View reviewed changes
assets/tests/hooks/useApiCall.test.ts
reject = rej
})

if (resolve === undefined || reject === undefined) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm trying to understand this. Will this non-deterministically detect a race condition (or something) where resolve or reject get set to undefined?

Copy link
Member Author

Choose a reason for hiding this comment

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

This was just to make typescript happy since I didn't have access to Promise.withResolvers

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't actually know why I don't have access to that, especially in a test environment, but I wasn't getting anywhere and didn't want to spend more time on it. I assume it has something to do with the package.json or something.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh uh, I don't think I need this anymore since I tried to give resolve and reject default values to try and get rid of this if statement. Not sure what would be better.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oooh I see - it's to get Typescript to recognize those values as not-undefined!!

This is wounding my brain, but is there any way to know that the function passed in to Promise's constructor will get called before the if statement happens?

I guess I dunno whether the if statement is better, or whether the default values are better... if there's some surprise and resolve/reject don't get initialized, then the test will be easier to debug with the if statement (cause it'll fail there), although I would perhaps update the message to be something more verbose than Error("Bad") 😂

Copy link
Member Author

Choose a reason for hiding this comment

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

😭

const PromiseWithResolvers = <T>() => {
  let resolve: undefined | ((v: T) => void) = undefined
  let reject: undefined | ((v: any) => void) = undefined
  const promise = new Promise<T>((res, rej) => {
    resolve = res
    reject = rej
  })

  if (resolve === undefined || reject === undefined) {
    throw Error("Bad")
  }

  return { promise, resolve, reject }
}

    tests/hooks/useApiCall.test.ts:130:7 - error TS2349: This expression is not callable.
      Type 'never' has no call signatures.

    130       resolve("first result")

Copy link
Member Author

Choose a reason for hiding this comment

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

Gosh I wish we had Promise.withResolvers 😞

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay actually I ended up fixing this! https://github.com/mbta/skate/compare/3628116696376489700efc70f4d5af4bf2057657..1c5eb538128a89c5f662396f834f5c373e278e60

@github-actions GitHub Actions
Copy link

Coverage of commit bc8891e

Summary coverage rate:
  lines......: 94.3% (3218 of 3413 lines)
  functions..: 73.9% (1333 of 1804 functions)
  branches...: no data found

Files changed coverage rate: n/a

Download coverage report

@firestack firestack force-pushed the kf/jj/znvltrqptqmy branch from bc8891e to 4ce2ae1 Compare April 11, 2024 21:15
@github-actions GitHub Actions
Copy link

Coverage of commit 4ce2ae1

Summary coverage rate:
  lines......: 94.3% (3218 of 3413 lines)
  functions..: 73.9% (1333 of 1804 functions)
  branches...: no data found

Files changed coverage rate: n/a

Download coverage report

@firestack firestack force-pushed the kf/jj/znvltrqptqmy branch from 4ce2ae1 to b767ac5 Compare April 11, 2024 21:53
@github-actions GitHub Actions
Copy link

Coverage of commit b767ac5

Summary coverage rate:
  lines......: 94.3% (3220 of 3415 lines)
  functions..: 73.9% (1333 of 1804 functions)
  branches...: no data found

Files changed coverage rate: n/a

Download coverage report

@firestack firestack force-pushed the kf/jj/znvltrqptqmy branch from b767ac5 to 78a697d Compare April 11, 2024 22:03
@github-actions GitHub Actions
Copy link

Coverage of commit 78a697d

Summary coverage rate:
  lines......: 94.3% (3220 of 3415 lines)
  functions..: 73.9% (1333 of 1804 functions)
  branches...: no data found

Files changed coverage rate: n/a

Download coverage report

@firestack firestack mentioned this pull request Apr 12, 2024
Merged
@firestack firestack force-pushed the kf/jj/znvltrqptqmy branch from 78a697d to 3628116 Compare April 12, 2024 17:58
@github-actions GitHub Actions
Copy link

Coverage of commit 3628116

Summary coverage rate:
  lines......: 94.3% (3221 of 3415 lines)
  functions..: 73.9% (1333 of 1804 functions)
  branches...: no data found

Files changed coverage rate: n/a

Download coverage report

@firestack firestack force-pushed the kf/jj/znvltrqptqmy branch from 3628116 to 1c5eb53 Compare April 16, 2024 18:07
@github-actions GitHub Actions
Copy link

Coverage of commit 1c5eb53

Summary coverage rate:
  lines......: 94.3% (3219 of 3415 lines)
  functions..: 73.8% (1332 of 1804 functions)
  branches...: no data found

Files changed coverage rate: n/a

Download coverage report

@firestack firestack merged commit 2b46f19 into main Apr 16, 2024
9 checks passed
@firestack firestack deleted the kf/jj/znvltrqptqmy branch April 16, 2024 20:15
This was referenced Apr 29, 2024
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joshlarson joshlarson joshlarson approved these changes

@hannahpurcell hannahpurcell Awaiting requested review from hannahpurcell

Assignees

@firestack firestack

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@firestack @joshlarson @hannahpurcell