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

feat: warn for chaning the config.suspense option during the lifecycle #898

Closed
wants to merge 1 commit into from
Closed

feat: warn for chaning the config.suspense option during the lifecycle #898

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
16 changes: 16 additions & 0 deletions src/use-swr.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -221,6 +221,8 @@ const mutate: mutateInterface = async (
return data
}

let didWarnChangingConfigSuspense = false

function useSWR<Data = any, Error = any>(
key: keyInterface
): responseInterface<Data, Error>
Expand Down Expand Up @@ -271,6 +273,20 @@ function useSWR<Data = any, Error = any>(
configRef.current = config
})

if (process.env.NODE_ENV !== 'production') {
Copy link
Member

@huozhi huozhi Jan 16, 2021 •
edited

Choose a reason for hiding this comment

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

🤔 not sure if we want to include process or more development message in the bundle. process will require polyfill to make it run inside browser, and more wranings will increase bundle size. currently swr isn't like react have 2 separate builds for dev and prod.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@huozhi Thank you for your feedback!
To be clear, React has two separate builds for dev and prod, but the entry point is one.
https://unpkg.com/browse/react@17.0.1/index.js

React depends on process.env.NODE_ENV to determine which build is used, so developers have to replace process.env.NODE_ENV at the build time.
https://reactjs.org/docs/optimizing-performance.html#webpack

For example, webpack handles the value based on the mode setting.
https://webpack.js.org/guides/production/#specify-the-mode
Rollup also has a way to handle it.

A polyfill for process is not required and is not included in the bundle because it is replaced with the actual string value like development or production. But developers don't have to do anything to handle the development warnings for SWR because they have to deal with process.env.NODE_ENV for React. This is often processed by libraries like Next.js, so they don't have to care about it in many cases.

The blocks in if (process.env.NODE_ENV !== 'production') { /* here */} are included in the published file of SWR, but they are eliminated in the production builds like developer warnings in React.
This is the way to be used in other libraries for React.

Copy link
Member

Choose a reason for hiding this comment

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

react can leverage NODE_ENV and split dev & prod into 2 builds cause it's using commonjs and dynamically require different builds based on NODE_ENV. and polyfill is a required action been taken on bundler side. esmodule cannot achive this.

  • if we include these conditions, either we kept them in the final bundle to let user polyfill node env and erase it by bundler; swr bundle size increase, final compressed bundle on userland is small but size is unknown.
  • or we only ship it with them eliminated, then user cannot leverage the warnings.

if the bundle size is a concern for swr cc @shuding

Copy link
Collaborator Author

@koba04 koba04 Jan 16, 2021 •
edited

Choose a reason for hiding this comment

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

@huozhi Thank you for your comment. I might not have understood your concern.
Does the "polyfill" you mentioned means that replacing process.env.NODE_ENV with the value like "production", or applyin polyfilll library like https://github.com/defunctzombie/node-process?

esmodule cannot achive this.

Why can't ESModule achieve this? This happens on the build time. Is it about conditional requires React does? If so, that's right, but we don't have to separate build files for dev and prod to eliminate the warnings.

if we include these conditions, either we kept them in the final bundle to let user polyfill node env and erase it by bundler; swr bundle size increase, final compressed bundle on userland is small but size is unknown.

Yes, if SWR doesn't want to increase file size even if this will be reduced in production build for browsers, SWR shouldn't add warning messages for developers.
This is a trade-off between a file size and developer experience. But end users don't affect the file size because the file size of a production build would keep small. Should we have to concern about the file size of SWR package itself?

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for raising these concerns. I’m thinking that if it’s possible to achieve what we want by publishing a custom eslint plugin for swr?

(Sorry that I didn’t think of that option earlier)

Copy link
Member

Choose a reason for hiding this comment

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

I’m thinking that if it’s possible to achieve what we want by publishing a custom eslint plugin for swr?

👍 that's probably a good idea that we don't have to touch swr source code to achive this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks! A custom ESLint plugin is a good idea. I'm not sure whether it could cover all cases or not, but it's worth experimenting it.

I think a custom ESLInt plugin couldn't cover the case of initialSize because a constant initialSize value might be passed through its props to the option; It's totally valid, but it's hard to detect that it's a constant value.
#905 (comment)

So in addition to the experiment, what about adding notes into the documentation? Currently, there is no way to know this.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, we should document those anyways.

if (
!didWarnChangingConfigSuspense &&
configRef.current &&
typeof configRef.current.suspense !== 'undefined' &&
configRef.current.suspense !== config.suspense
) {
console.error(
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

console.warn might be better

'config.suspense should not be changed during the lifecycle'
)
didWarnChangingConfigSuspense = true
}
}

if (typeof fn === 'undefined') {
// use the global fetcher
fn = config.fetcher
Expand Down
44 changes: 43 additions & 1 deletion test/use-swr.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1927,7 +1927,11 @@ describe('useSWR - local mutation', () => {
})

describe('useSWR - configs', () => {
afterEach(cleanup)
afterEach(() => {
jest.clearAllMocks()
jest.restoreAllMocks()
cleanup()
})

it('should read the config fallback from the context', async () => {
let value = 0
Expand Down Expand Up @@ -2007,6 +2011,44 @@ describe('useSWR - configs', () => {
await act(async () => await new Promise(res => setTimeout(res, 400)))
expect(container.firstChild.textContent).toMatchInlineSnapshot(`"data: 1"`)
})

it('should warn for changing the config.suspense option during the lifecycle', async () => {
function Page() {
const [suspense, setSuspense] = useState(true)
const { data } = useSWR('config-2', () => 'data', {
suspense
})
return (
<div onClick={() => setSuspense(!suspense)}>
{data}:{`${suspense}`}
</div>
)
}

jest.spyOn(console, 'error').mockImplementation(() => {})

const { container } = render(<Page />)
expect(container.firstChild.textContent).toMatchInlineSnapshot(
`"data:true"`
)

fireEvent.click(container.firstElementChild)
await waitForDomChange({ container })
expect(container.firstChild.textContent).toMatchInlineSnapshot(
`"data:false"`
)
expect(console.error).toBeCalledWith(
'config.suspense should not be changed during the lifecycle'
)
expect(console.error).toBeCalledTimes(1)

fireEvent.click(container.firstElementChild)
expect(container.firstChild.textContent).toMatchInlineSnapshot(
`"data:true"`
)
// console.error shouldn't be called twice
expect(console.error).toBeCalledTimes(1)
})
})

describe('useSWR - suspense', () => {
Expand Down