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

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

Closed

Conversation

koba04
Copy link
Collaborator

@koba04 koba04 commented Jan 13, 2021

This PR follows up #886 (comment).

This adds a warning for changing the config.suspense option during the lifecycle. Printing the warning on every render is noisy, so this prints the warning only once, which is a technique used in the internal of React.

This also introduces the concept of development warnings. The implementation depends on the value of process.env.NODE_ENV, which is the same way that React does. swr is a library for React hooks, so I think swr can depend on the same way with React.
https://reactjs.org/docs/optimizing-performance.html#use-the-production-build
Should I mention process.env.NODE_ENV in the documentation?

I think the concept of development warnings makes swr more developer-friendly because it makes it possible to add more warnings without affecting production builds.

@codesandbox-ci
Copy link

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 cb15d39:

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

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

@@ -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

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

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.

@koba04
Copy link
Collaborator Author

koba04 commented Jan 16, 2021

I should test this after #881 has been merged.
In addition, I should also address #905 (comment) as another PR.

@koba04
Copy link
Collaborator Author

koba04 commented Jan 17, 2021

I'll close and take alternative ways not adding warnings in SWR.

@koba04 koba04 closed this Jan 17, 2021
@koba04 koba04 deleted the warn-for-changing-config-suspense branch January 17, 2021 13:00
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