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.

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

Middleware #1160

Merged
merged 18 commits into from
Jun 21, 2021
Merged

Middleware #1160

merged 18 commits into from
Jun 21, 2021

Conversation

shuding
Copy link
Member

@shuding shuding commented May 9, 2021

This is an implementation of #875, #172, #1030.

Middlewares

The new option config.middlewares is now supported, for example:

<SWRConfig value={{
  middlewares: [
    (useSWRNext) => (key, fn, config) => {
      console.log('fetching', key, '...')
      return useSWRNext(key, fn, config)
    }
  ]
}}>

Middlewares can be extended via multiple contexts and/or per-hook config, for example:

<SWRConfig value={{ middlewares: [A] }}>
  <SWRConfig value={{ middlewares: [B] }}>
    ...
      useSWR(..., { middlewares: [C] }
...

// Equivalent to `middlewares: [A, B, C]`.

Middlewares will be executed from inner to outer (this will be extremely helpful for custom SWR hooks that override key, fn, or config. So higher level middlewares don't need to care about the details):

<SWRConfig value={{ middlewares: [A, B, C] }}>

// C, B, A will be called successively.

Concerns

  • Which key should be passed to the middlewares (original or serialized)? Original, so the middleware will have more flexibility.
  • Is middlewares a good name? How about plugins/use?

Notes

This PR refactors some internal APIs such as useArgs. Those will simplify the process of building other hooks on top of SWR.

Also, the ESLint rule react/prop-types is now disabled given we have TypeScript already.

Credits to https://redux.js.org/understanding/history-and-design/middleware and #1030 (comment) for the idea and implementation discussions.

@codesandbox-ci
Copy link

codesandbox-ci bot commented May 9, 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 a81bc8c:

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

@shuding shuding mentioned this pull request May 10, 2021
4 tasks
@shuding
Copy link
Member Author

shuding commented May 20, 2021

I think the original key should be passed to middlewares...

@shuding shuding marked this pull request as ready for review June 17, 2021 08:12
@shuding shuding requested a review from huozhi June 17, 2021 08:13
.eslintrc Outdated Show resolved Hide resolved
src/index.ts Show resolved Hide resolved
src/config-context.ts Outdated Show resolved Hide resolved
Copy link
Member

@huozhi huozhi left a comment

Choose a reason for hiding this comment

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

🚀

Copy link
Collaborator

@koba04 koba04 left a comment

Choose a reason for hiding this comment

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

Great work 👏 👏
I've found some type errors in the middleware tests and fixed type errors and added types to middlewares in tests.

I'm also confused the behavior for multiple middlewares and modifying keys.
The following test now fails. Isn't it possible to modify keys in middlewares?

  it('should pass modified keys to the next middlewares and useSWR', async () => {
    const key = createKey()
    const createDecoratingKeyMiddleware = (c: string): Middleware => useSWRNext => (k, fn, config) => {
      return useSWRNext(`${c}${k}${c}` , fn, config)
    }
    function Page() {
      const { data } = useSWR(key, () => {
        return createResponse(key)
      }, {
        middlewares: [createDecoratingKeyMiddleware('!'), createDecoratingKeyMiddleware('#')]
      })
      return <div>hello, {data}</div>
    }

    render(<Page />)
    screen.getByText('hello,')
    await screen.findByText(`hello, #!${key}!#`) // actual: the key is not decorated
  })

src/types.ts Outdated Show resolved Hide resolved
test/use-swr-middlewares.test.tsx Outdated Show resolved Hide resolved
test/use-swr-middlewares.test.tsx Outdated Show resolved Hide resolved
test/use-swr-middlewares.test.tsx Outdated Show resolved Hide resolved
test/use-swr-middlewares.test.tsx Outdated Show resolved Hide resolved
test/use-swr-middlewares.test.tsx Outdated Show resolved Hide resolved
test/use-swr-middlewares.test.tsx Outdated Show resolved Hide resolved
@koba04
Copy link
Collaborator

koba04 commented Jun 19, 2021

I've also created a sample middleware for the status middleware you mentioned in #1215 (comment)

I could implement it, but it was difficult to extend the return type of useSWR (please see the @ts-ignore in the following code). so we might need to add a way to extend the SWRResponse type if SWR covers the use-case.

  it('should pass modified keys to the next middlewares and useSWR', async () => {
    const statusMiddleware: Middleware = useSWRNext => (k, fn, config) => {
      const result = useSWRNext(k, fn, config);
      return {
        ...result,
        status: result.data ? 'ready': 'fetching'
      }
    }
    function Page() {
      // @ts-ignore the status property should be typed.
      const { data, status } = useSWR('key', () => {
        return createResponse('data')
      }, {
        middlewares: [statusMiddleware]
      })
      return <div>{status}:hello, {data}</div>
    }

    render(<Page />)
    screen.getByText('fetching:hello,')
    await screen.findByText(`ready:hello, data`)
  })

shuding and others added 2 commits June 20, 2021 23:59
Co-authored-by: Toru Kobayashi <koba0004@gmail.com>
Co-authored-by: Toru Kobayashi <koba0004@gmail.com>
@shuding
Copy link
Member Author

shuding commented Jun 20, 2021

Thanks for contributing another test case, Toru!

The following test now fails. Isn't it possible to modify keys in middlewares?

I think that's because the fetcher directly returned the original key from the outer context:

useSWR(key, () => {
  return createResponse(key)
})

If we change it to this, it will work:

useSWR(key, (k) => {
  return createResponse(k)
})

Also, the result should be !#${key}#! instead of #!${key}!#, because new middlewares will be applied first.

Co-authored-by: Toru Kobayashi <koba0004@gmail.com>
@shuding
Copy link
Member Author

shuding commented Jun 20, 2021

So we might need to add a way to extend the SWRResponse type if SWR covers the use-case.

Yeah absolutely. Ideally the config of custom SWR hooks should be extendable too... I don't know if there's a good way to do it with TS though.

@koba04
Copy link
Collaborator

koba04 commented Jun 21, 2021

I think that's because the fetcher directly returned the original key from the outer context:

I misunderstood it. Thank you!

Yeah absolutely. Ideally the config of custom SWR hooks should be extendable too... I don't know if there's a good way to do it with TS though.

The easiest way to do it is to add additional type parameters to useSWR, but I don't think it's the best way to achieve it, so I'll considering another way for it.

    function Page() {
      type CustomSWRResponse = SWRResponse<string, any> & { status: string }
      const { data, status } = useSWR<string, any, CustomSWRResponse>('key', () => {
        return createResponse('data')
      }, {
        middlewares: [statusMiddleware]
      })
      return <div>{status}:hello, {data}</div>
    }

I don't think it's a blocker to merge this, so feel free to go forward👍

@shuding
Copy link
Member Author

shuding commented Jun 21, 2021

Yeah, I think it would be very tricky to directly extend the types of key, fn, config, and return value of useSWR. Maybe for now we recommend building custom hooks on top of useSWR, which handles type issues with the custom hook itself.

@shuding shuding merged commit 3923fd3 into master Jun 21, 2021
@shuding shuding deleted the middleware branch June 21, 2021 20:36
@shuding shuding added this to the 1.0 milestone Jun 30, 2021
@corysimmons corysimmons mentioned this pull request Sep 8, 2021
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.

None yet

3 participants