Skip to content

Export default SWR config to allow more flexible extensions #1023

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.

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

Merged
merged 5 commits into from
Mar 16, 2021
Merged

Export default SWR config to allow more flexible extensions #1023

merged 5 commits into from
Mar 16, 2021

Conversation

jakubriedl
Copy link
Contributor

@jakubriedl jakubriedl commented Mar 10, 2021

To allow more flexible customisations and extensions it would be useful to export default config.

Eg. in our case we want to extend onErrorRetry but keep the current logic. Now we had to copy-paste the default config to our wrapper instead of calling it.

@codesandbox-ci
Copy link

codesandbox-ci bot commented Mar 10, 2021

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 169909c:

Sandbox Source
SWR-Basic Configuration
SWR-States Configuration
SWR-Infinite Configuration

@huozhi
Copy link
Member

huozhi commented Mar 12, 2021

I feel it would be more convenient to change value of SWRConfig to support function, with defaultConfig as argument. to avoid config argument in callbacks been overridden.

<SWRConfig 
  value={(defaultConfig) => {
   return {
     onErrorRetry(...args) {
        // customization
        defaultConfig.onErrorRetry(...args)
     }
   }
  }}
>
 <App />
</SWRConfig>

Then you can customize the callbacks at the top root.

@jakubriedl
Copy link
Contributor Author

Thanks for suggestion @huozhi, in normal application it makes sense.

Our use case is bit different though. We are building our own reusable hook that wraps useSWR() and adds bunch of custom logic & features to it. So when other teams within company use it they don't (have to) know that it is using swr under the hood so wrapping the app in <SWRConfig> would get very impractical. Our config is still static (doesn't change during runtime) it is just extension of the existing one.

@huozhi
Copy link
Member

huozhi commented Mar 13, 2021

I see your point, current accessing defaultConfig per hook is not ideal.

IMO we can add property config.defaults into useSWR's config value representing the defaultConfig, it's static and not overridable. Since there's not too much pre configured stuff in default config, exposing one more API/variable would be heavy.

// use-swr.ts
const config = Object.assign(
    {},
    defaultConfig,
    useContext(SWRConfigContext),
    args.length > 2
      ? args[2]
      : args.length === 2 && typeof args[1] === 'object'
      ? args[1]
      : {}
+   { defaults: defaultConfig }
  )

then you're able to customize it through hook options like beblow. This can also keep the exposed API as minimal as possible.

useSWR(key, fn, {
  onErrorRetry(err, key, config) {
    const defaultOnErrorRetry = config.defaults.onErrorRetry
    // customization
  }
})

@shuding
Copy link
Member

shuding commented Mar 13, 2021

-1 on config.defaults, since that will be circular dependency (semantically). If we do want to expose it, making it a static property of the config context SWRConfig.default would be better. Also the singular name default feels better personally.

@jakubriedl
Copy link
Contributor Author

Both options would work for us, so don't mind which one you decide to go with.
On the other hand the SWRConfig.default static prop sounds "cleaner" to me, but thats completely your call.

Do you want me to go and update this PR that way or will you do it by your own?

@huozhi
Copy link
Member

huozhi commented Mar 14, 2021

@jakubriedl go ahead with SWRConfig.default in this PR, we'll adopt it once you finished 👍

@jakubriedl
Copy link
Contributor Author

is this the way you expected @huozhi ?

@shuding shuding merged commit 7f490c1 into vercel:master Mar 16, 2021
@shuding
Copy link
Member

shuding commented Mar 16, 2021

Thanks @jakubriedl! Will publish a pre-release soon.

@jakubriedl jakubriedl deleted the patch-1 branch March 17, 2021 03:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants