Skip to content

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

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

[RFC] Plugin / middleware #172

Closed
shuding opened this issue Dec 4, 2019 · 18 comments
Closed

[RFC] Plugin / middleware #172

shuding opened this issue Dec 4, 2019 · 18 comments
Assignees
Labels
discussion Discussion around current or proposed behavior RFC
Milestone

Comments

@shuding
Copy link
Member

shuding commented Dec 4, 2019

A plugin (or middleware) can extend the configuration, key, and fetcher function of SWR. With this feature we can make SWR lightweight and more customizable.

API

import swrAxios from 'swr-axios'
import useSWR from 'swr'

// ...
useSWR('key', fetcher, { use: swrAxios })

You can use multiple plugins too:

useSWR('key', fetcher, { use: [...plugins] }

Or pass them to the context:

<SWRConfig value={{ use: [...plugins] }}>

Implementation Details

swrAxios should be a function, which accepts all the params passed to the SWR hook, and return the extended ones:

function swrAxios (key, fetcher, config) {
  ...
  return [newKey, newFetcher, newConfig]
}

That way, middleware can be chained, and you will also be able to use custom configs too (because they'll be passed to the handlers).

Ideas

  • swr-fetch (with fetch polyfill, or isomorphic-fetch, etc.)
  • swr-logger (middleware for logging network requests)
  • swr-axios, swr-got, swr-graphql ... (wrapped with custom data fetching libs)
  • swr-local-storage (custom cache backend)
  • swr-error-handler (e.g. redirect to /login on 404 etc.)
  • swr-state (global local state)
  • swr-socketio
    ...

Related Feature Requests

@shuding shuding added the discussion Discussion around current or proposed behavior label Dec 4, 2019
@o-alexandrov
Copy link

@quietshu
Great idea. Thank you.
It seems converting this repo to a monorepo first to contain at least official plugins like swr-axios should be the task prior to this one.

@sergiodxa
Copy link
Contributor

I'm not sure but I think this could happen on user-land? Create a useExtendedSWR on your project or publishing it to npm and that will work as a middleware/plugin.

@o-alexandrov
Copy link

Leaving swr-axios on the user-land would mean the necessity to manage multiple repos, making it more difficult to keep the plugins up-to-date, given the fast pace of features being added to swr.

I believe, in any typical frontend project that works with a RESTful API, you would expect to see either axios or request libs to be used to send HTTP requests.

  • adding these two libs as official plugins makes sense for me

If you guys agree, I can provide with a separate PR to make it a monorepo.

@sergiodxa
Copy link
Contributor

I don't mean making swr-axios in user land, I mean don't adding support for this since it could happen in user land.

function useSWRAxios(...args) {
  // do something here to extend or reduce args
  return useSWR(...newArgs)
}

It's not hard to write a wrapper around a hook creating a new hook and support composition.

@o-alexandrov
Copy link

@sergiodxa I believe you missed the related issues above that indicate a more complicated use case, especially for axios.

I participate in discussions in other libs like react-async, react-query.
The major concern people have is the complicated scenarios with query string params, etc. that would make the work with a key param of useSWR complicated.
That's why @Svish introduced his idea with JSON.stringify of the axios request and I discussed a more general approach that everyone could build on top #166 (comment)

@o-alexandrov
Copy link

o-alexandrov commented Dec 5, 2019

@sergiodxa sorry for the noise, I believe I didn't provide enough arguments, so let me reply to you:

It's not hard to write a wrapper around a hook creating a new hook and support composition.

Look at the amount of work done in #145
And the funny thing, that solution is still imperfect, as well as my proposal also has issues #166 (comment)

Why not to combine efforts to seek the best practices, instead of everyone creating the same imperfect solutions over and over in their own repos?

@sergiodxa
Copy link
Contributor

sergiodxa commented Dec 5, 2019

🤔 I think I get it, I'm still unsure about having plugins support in useSWR, maybe this repo could be a monorepo as you said and have two or more libs:

  • swr
  • swr-axios
  • swr-*

So if you want only SWR you yarn add swr, but if you want to use it with axios you could install yarn add swr swr-axios and if you want SWR with another integration you could yarn add swr swr-*.

And all of these extra packages could be custom hooks using useSWR internally (they could set swr as peer dependency too) so you don't need custom plugins support in SWR, compose the hooks creating new hooks, this way you don't need to add an extra feature to SWR, and you could still have a better single implementation of how to integrate SWR with Axios (and other fetching libs).

Also those swr-* could not only be here, but be in other repos, so you could publish your own wrapper around SWR to integrate it with another library, as a lot of Redux libraries did, eventually officially supported implementations could be moved to this repo or be recommended in the README.

What do you think? @o-alexandrov @quietshu

I'm mostly worry about adding a feature that could not be really necessary since hooks allow composability and specially it's probably not going to be used by ZEIT, I like that SWR comes from ZEIT internal usage since that guarantees me you are going to keep maintaining the lib and those features.

@o-alexandrov
Copy link

@sergiodxa
This is exactly what I would love to see in this project to happen.
@quietshu I could propose a PR with a monorepo setup of lerna + yarn workspaces:

@sergiodxa I believe you have a typo:

maybe this repo could be a monorepo as you said and have two libs:

@shuding shuding added the RFC label Feb 24, 2020
@GiancarlosIO
Copy link

👀

@shuding shuding mentioned this issue Jun 6, 2020
@shuding shuding self-assigned this Sep 20, 2020
@shuding shuding added this to the 1.0 milestone Sep 20, 2020
@matamatanot
Copy link
Contributor

matamatanot commented Oct 30, 2020

What's going on with this issue?
Allow me to tell you about my complicated pattern. I use SWR with ky. request<T> extends kyHttpClient for firebase.User and locale and so on.

https://github.com/sindresorhus/ky/blob/764bc1cdabdb65bbcd9830b0f8bfbd95fed3179a/index.d.ts#L159
// ky’s option
type RequestOptions = Options;

type AdditionalRequestOptions = {
  locale?: ja | en;
  firebaseUser?: firebase.User;
  forceRefreshToken?: boolean;
};

/**
 * For auth required endpoints
 */
export const useSWRRequestWithFirebaseUser = <T>(
  url: string | null,
  options: RequestOptions,
  additionalOpts: AdditionalRequestOptions,
  swrConfig?: CustomSwrConfig<T>
) => {
  const { locale } = additionalOpts;
  return useSWR<T>(
    url && additionalOpts?.firebaseUser
      ? `${url}${JSON.stringify({ locale, ...options })}`
      : null,
    () => request<T>(url || "", options, additionalOpts),
    swrConfig
  );
};

/**
 * For auth optional endpoints
 */
export const useSWRRequestWithOrWithoutFirebaseUser = <T>(
  url: string,
  options: RequestOptions,
  additionalOpts: AdditionalRequestOptions,
  swrConfig?: CustomSwrConfig<T>
) => {
  const { locale } = additionalOpts;
  return useSWR<T>(
    `${url}${JSON.stringify({ locale, ...options })}`,
    () => request<T>(url, options, additionalOpts),
    swrConfig
  );
};

Using SWR with HttpClient and Auth library become complicated. 😭

@shuding
Copy link
Member Author

shuding commented Oct 31, 2020

Thank you for sharing your use case here @matamatanot, it’s very helpful!

I think instead of using a middleware, we can think about adding a new option serialize:

useSWR([url, opts], fetcher, { serialize: true })

@matamatanot
Copy link
Contributor

matamatanot commented Oct 31, 2020

@shuding Thanks! A new option serialize seem to be very useful. I would love to see the option added to the library.

But, I think there is a better way.
firebase.User is a little large object. Serializing a firebase.User object go too far. For many people, what we want to achieve is that if a user logs out or switches to another account, the cache will not be returned.

@shuding
Copy link
Member Author

shuding commented Oct 31, 2020

I see @matamatanot! While serialize is helpful, what we can do is to provide another option disable, which is an alternative to useSWR(null, fetcher) like what you are doing.

@koba04
Copy link
Collaborator

koba04 commented Feb 12, 2021

This is an interesting proposal.
I don't fully understand the goal of the middleware system.
But the following also might be an option for the interface.

const plugin = next => (key, fn, config) => {
  // preprocessing
  const result = next(key, fn, config) // a chained middleware or useSWR
  // postprocessing
  return result
}

The interface, which this PR proposes, has to override the fetcher implementation in many cases like logging. Personally, I feel it seems to be more clear to be able to add plugin implementations around the original useSWR.
The interface is similar to the Redux middleware, so some developers might be already familiar with it.

The following is my experimentation with the interface.
https://github.com/koba04/swr/blob/a23416532c35364b54dfbb7b235176b0b61a7313/test/use-swr-middleware.test.tsx#L8-L22
It's just proof of concept, so it doesn't cover many cases like SWRConfig and so on.

I think it depends on the use-case what the appropriate interface for the middleware system is, so this is just an idea.

@shuding
Copy link
Member Author

shuding commented Mar 7, 2021

@koba04 Love your idea and demo, the support of postprocessing is so great! I think we can implement this.

I don't fully understand the goal of the middleware system.

The main goal is to build tooling around SWR, because data fetching is not just getting the data. For example in our particular use case we made a logger function that wraps the fetcher, so every request will be logged with its status. I believe that many people have built things like that repeatedly, and it will be great that SWR can provide a simple and lightweight middleware support so these things can be shared.

@Svish
Copy link
Contributor

Svish commented Mar 7, 2021

a logger function that wraps the fetcher

Why would you need a complicated middleware feature for that though, if it's just a matter of making a wrapped fetcher function which does the logging?

@shuding
Copy link
Member Author

shuding commented Mar 7, 2021

@Svish because that's not going to scale. The logger wrapper needs to be implemented differently for fetch, axios, graphql, ... And that's just 1 use case.

And also I believe this is not a "complicated feature", it's intuitive to me:

import logger from 'swr-logger'
import { SWRConfig } from 'swr'

<SWRConfig value={{ use: logger }}>
  <App/>
</SWRConfig>

@huozhi
Copy link
Member

huozhi commented Mar 12, 2021

Proposed API

type SWRPluginNext = (key: Key, fn: Fetcher, config?: SWRConfiguration) => void
type SWRPlugin = (next: SWRPluginNext) => void

Example

const plugin = next => (key, fn, config) {
   // ...
}

Usages

1. Add auth token

const userToken = resolveUserToken()

functuion fetcher(key, token) {
  return fetch(key, {
     headers: { 'X-User-Token': token }
  })
}

const authFetcherPlugin = (next) => (key, fn, config) => {
   const authedFetcher = (key) => fn(key, userToken)

   return next(key, authedFetcher, config)
}

This can help simplify the redudant array key use cases in useSWR when values in array are for fetcher only.

2. Error reporting

const errorReportPlugin = (next) => async (key, fn, config) => {
  functuion fetcher(...args) {
     let data, error
     try {
       data = fn(...args)
     } catch (err) {
       error = err
     }
     if (error) {
       sentry.reportError(error)
       throw error
     }
     return data
   }

   next(key, fetcher, config)
}

I guess the solution could either be wrap swr with HOC or just wrap the fetcher to process data.

@shuding shuding closed this as completed Mar 12, 2021
@vercel vercel locked and limited conversation to collaborators Mar 12, 2021

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

Labels
discussion Discussion around current or proposed behavior RFC
Projects
None yet
Development

No branches or pull requests

8 participants