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

Bug: The loading state of useRequest is incorrect with react-refresh #390

Closed
BryceHQ opened this issue May 11, 2020 · 16 comments · Fixed by #943
Closed

Bug: The loading state of useRequest is incorrect with react-refresh #390

BryceHQ opened this issue May 11, 2020 · 16 comments · Fixed by #943
Assignees
Labels
bug Something isn't working
Milestone

Comments

@BryceHQ
Copy link

BryceHQ commented May 11, 2020

React version: 16.13.1
react-refresh version: 0.8.1
react-refresh-webpack-plugin version: 0.3.0-beta.6
umi/hooks version: 1.9.2

Steps To Reproduce

  1. run react-refresh-webpack-plugin/examples/webpack-dev-server
  2. useRequest in examples/webpack-dev-server/src/FunctionNamed.js
  3. try hmr

Link to code example:

import React from 'react';
import { useRequest } from '@umijs/hooks';

function getUsername() {
  return new Promise(resolve => {
    setTimeout(() => {
      resolve('test');
    }, 1000);
  });
}

export function FunctionNamed() {
  const { data, loading } = useRequest(getUsername)
  console.log('x', loading);
  return <h1>
    {loading ? 'loading' : data}
  </h1>;
}

The current behavior

The result of loading will always be true when hmr.

The expected behavior

The result of loading should be the right value.

The bug reason

The state of Function component is preserved, but effects re-run in React Refresh.
For Detail.

A suggestion for code shown as below :

refresh() {
    if (this.unmountedFlag) this.unmountedFlag = false;
    return this.run(...this.state.params);
}

Thanks for your help.

@smajl
Copy link

smajl commented May 25, 2020

This is mega annoying when you are using react-refresh... you have to either reload the whole page every time or disable reading loading prop during development... Fix would be so appreciated.

@nemesisqp
Copy link

data get stuck after hmr happened too

@awmleer awmleer added bug Something isn't working and removed Primary labels Jul 8, 2020
@awmleer awmleer added this to the v2.3.0 milestone Jul 8, 2020
@brickspert
Copy link
Collaborator

brickspert commented Jul 21, 2020

This is a bug for react-refresh-webpack-plugin.

It will not work correctly in this scenario

import React from 'react';

function getUsername() {
  return new Promise(resolve => {
    setTimeout(() => {
      resolve('test');
    }, 1000);
  });
}

export function FunctionNamed() {

  const isUnmountFlag = React.useRef(false);
  const [loading, setLoading] = React.useState(true);

  React.useEffect(() => {
    setLoading(true);
    getUsername().then(() => {
      if (!isUnmountFlag.current) {
        setLoading(false);
      }
    });

  }, []);

 // it will trigger unmount first
  React.useEffect(() => {
    return () => {
      isUnmountFlag.current = true;
    }
  });

  return <h1>
    ...
    {loading ? 'loading' : 'data'}
  </h1>;
}

@BryceHQ
Copy link
Author

BryceHQ commented Sep 2, 2020

大哥们,这个问题还是没有解决啊。gaearon 已经说明了这不是bug,是特性。pmmmwh/react-refresh-webpack-plugin#85 (comment)

@brickspert
Copy link
Collaborator

大哥们,这个问题还是没有解决啊。gaearon 已经说明了这不是bug,是特性。pmmmwh/react-refresh-webpack-plugin#85 (comment)

就是它们的 bug,useRequest 没法兼容~~

@BryceHQ
Copy link
Author

BryceHQ commented Sep 3, 2020

为啥呢?我之前也这样觉得,所以也给他们提了bug,但是 Dan 解释之后,我觉得他的说法也有道理。至少目前 react 官方认为这是特性不是bug。

@brickspert
Copy link
Collaborator

为啥呢?我之前也这样觉得,所以也给他们提了bug,但是 Dan 解释之后,我觉得他的说法也有道理。至少目前 react 官方认为这是特性不是bug。

其实就像我前面写的这个 demo,在 react-refresh 里面,明显执行的就有问题。

@BryceHQ
Copy link
Author

BryceHQ commented Sep 3, 2020

为啥呢?我之前也这样觉得,所以也给他们提了bug,但是 Dan 解释之后,我觉得他的说法也有道理。至少目前 react 官方认为这是特性不是bug。

其实就像我前面写的这个 demo,在 react-refresh 里面,明显执行的就有问题。

仔细看下我给 react 提的 issue 呗。你的例子和我的例子没有本质区别。
我觉得 Dan 的解释是能说的通的,是现在这种 unmount 的用法和 hooks 本身的设计不一致。

Try to stop thinking about effects as "mount" and "unmount". This isn't the right mental model for them. Effects are more about "synchronizing some values with some side effect". In that sense, it makes sense that we want to resynchronize them if the code changes.

@nanxiaobei
Copy link

nanxiaobei commented Sep 10, 2020

这个问题会有后续吗?
我觉得关键是,React 官方认为,useEffect 里 return 的函数并不是 componentWillUnmount
而是 componentWillUpdate

@brickspert
Copy link
Collaborator

这个问题会有后续吗?
我觉得关键是,React 官方认为,useEffect 里 return 的函数并不是 componentWillUnmount
而是 componentWillUpdate

暂时没有好的想法。

  • 如果监听不到组件的 unmount,一定会导致异步请求造成的内存泄漏。并没有其它方案能监听组件卸载。
  • react-refresh 的行为和 react 行为不一致,难道不是他们的 bug 么~~

@dpyzo0o
Copy link

dpyzo0o commented Nov 8, 2020

@brickspert 这个怎么 close 了呢?是否可以参考 react-query 是怎么处理的?我用 react-query 没有碰到这个问题

@MeetzhDing
Copy link

热更新状态下,loading一直为true,data的更新也会受到影响。开发的状态下几乎是不可用状态

@xialvjun
Copy link

facebook/react#21019

好像是 react-refresh 让 react 在 hmr rerender 的时候,重新执行了 空数组依赖的 useEffect。。。

空数组依赖的 useEffect 在 hmr 时不应重复执行。

@gaearon
Copy link

gaearon commented Mar 16, 2021

It's not a bug in React. It's intentional that useEffect will cleanup and re-setup during Fast Refresh: facebook/react#21019 (comment). The fix is to write effects in a more resilient way, which would be important for other features (not just Fast Refresh) in the future. Happy to answer other questions in the linked issue if you're struggling with some concrete pattern.

@turkyden
Copy link
Contributor

turkyden commented Mar 26, 2021

Hi, Dan @gaearon

@brickspert
Copy link
Collaborator

I have fixed in #943 ,
But i have some question for react-refresh's behavior in pmmmwh/react-refresh-webpack-plugin#384

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.