-
Notifications
You must be signed in to change notification settings - Fork 5
Conversation
* Add createQueryHook * Add createInfiniteHook * Add createImmutableHook * Add useMutate * Remove original exports * Upgrade dependencies * Revamp package setup * Remove old tests * Begin adding tests * Add mutate tests * Add infinite tests * Update tsconfig for builds * Tiny updates * Update package manager * Remove setup test data script * Apply lint and build fixes * Update README * Add git hooks for linting and formatting * Fix formatting * Fix exports * Fix lint errors and update readme * Remove tsconfig.build.json
"swr": "2" | ||
}, | ||
"peerDependenciesMeta": { | ||
"openapi-typescript": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How would types be generated without this? Is it really optional? I guess it could be used through CLI and not installed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is right. This library isn't in charge of type generation, which is why I add it as an optional peer. I might even be able to remove it altogether, but I'd need to double check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After digging deeper, I think leaving in openapi-typescript
as an optional peer dependency makes sense. Even though swr-openapi
does not require it, the types it uses are expected to have been generated by openapi-typescript@7
.
import { configureBaseQueryHook } from "./query-base.js"; | ||
|
||
/** | ||
* ```ts |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe worth adding a description to the function commands and a @see
which links to the SWR docs for this
@@ -0,0 +1,78 @@ | |||
import type { Client } from "openapi-fetch"; | |||
import type { MediaType, PathsWithMethod } from "openapi-typescript-helpers"; | |||
import useSWRInfinite, { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is infinite maybe worth putting it it's own slash path the same way SWR does? I ask because it seems like part of their v1 release did this with intention
Maybe not necessary just a thought since it is what they did
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually spent quite a while on trying to implement that, but ESM-only tree-shaking was much simpler to rely on. There are some strange hacks going on in swr (look in swr/dist/infinite
) that support their bundling, and I couldn't get it to work in consumers even when copying their methodology.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At the very least, if a consumer doesn't end up using the infinite hooks, they should be tree-shaken at build time.
import { configureBaseQueryHook } from "./query-base.js"; | ||
|
||
/** | ||
* ```ts |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May be worth linking to the useSWRImmutable docs here with a @see
|
||
/** | ||
* ```ts | ||
* import { isMatch } from "lodash"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't isMatch return true for partial object matches since it is just a check for object values? Then wouldn't that cause a query with one parameter being the same value, but not having the other values to show as the same key?
If that it the goal it may be worth clarifying that here with this example with something like "updates queries even if not all parameters provided"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is definitely the intent. I leave the comparator as customizable though, so the library doesn't actually require _.isMatch
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally looking good - just a few questions and callouts of where it might be nice to add clarity + links to comments
Summary
useImmutable
anduseMutate
hooks.createHooks
into separate builder functions.Breaking Changes
createHooks
with separate builder functions.matchKeyComparator
withuseMutate
.RequestTypes
→TypesForRequest
.Migration from v4
Imports
Replace
createHooks
calls withcreateQueryHook
,createInfiniteHook
, andcreateImmutableHook
as needed.Fetch Options
For requests made that don't have required fetch options (like a path parameter), feel free to remove empty init arguments like this, as they are no longer required!
Key Matchers
Remove
applySwrOpenApiConfiguration
and define auseMutate
hook usingcreateMutateHook
:To replace
matchKey
in consumers, calluseMutate
and invoke the callback like this:ESM in Next.js
If using Next.js in a non-ESM project, add
swr-openapi
to thetranspilePackages
array in your Next.js config file. This should take care of transpilation automatically, as well as transforming in Jest (if you usenext/jest
).