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

deprecated componentWillReceiveProps #25

Open
stropho opened this issue Apr 12, 2019 · 15 comments
Open

deprecated componentWillReceiveProps #25

stropho opened this issue Apr 12, 2019 · 15 comments

Comments

@stropho
Copy link

stropho commented Apr 12, 2019

This does NOT need an immediate attention, but ..
Earlier in some minor version of react@16, componentWillReceiveProps and a few other methods were flagged as deprecated. The intention is to remove them completely in react@17.

In this package, both componentWillReceiveProps and componentDidUpdate do a lot of stuff, and I have no idea how complex it could be (if actually possible) to replace it by the new lifecycle methods. Btw. there is a polyfill for the new methods in older versions of react https://github.com/reactjs/react-lifecycles-compat

Is there any plan to use the new lifecycle methods ?

As I said, this does not need to solved right now. However, it would be nice to have some idea about what's going to happen with this package once the release of react@17 is happening.

@patrik-piskay
Copy link
Owner

componentDidUpdate will be fine even in the future, but you are correct we need to think how to migrate componentWillReceiveProps - the easiest short term solution would be to rename it to UNSAFE_componentWillReceiveProps once cWRP is deprecated, but that will not work with the future React's async mode.

I like cWRP though, because we have a nice separation between prop updates (content that may need to be truncated, cWRP) and internal state updates (needed for the truncation, cDU)

@stropho
Copy link
Author

stropho commented Apr 14, 2019

The way you describe cWRP sounds like it could be replaced with getDerivedStateFromProps . It is a static function, so there is no access to this. That could make the migration more complicated. I might look into it some day. As it is not a pressing matter now, it might not be anytime soon 😉

Feel free to close this issue, or keep it as a reminder .. up to you!

@bitjoo
Copy link

bitjoo commented Sep 4, 2019

Hello everyone. Thank you for this nice little component.
What is the status here? Is something planned in the future to get around componentWillReceiveProps?

@patrik-piskay
Copy link
Owner

Yes @bitjoo, thanks for reminding me, I'll plan to take a look soon!

@patrik-piskay
Copy link
Owner

UNSAFE_componentWillReceiveProps released in v4.1.0

@stropho
Copy link
Author

stropho commented Apr 14, 2020

So I did some digging, unfortunately not found a clean solution, but at least I can share what I've found.

The TruncateMarkup component works in more or less those phases:

  1. receive an external update, such as in UNSAFE_componentWillReceiveProps or from resizeObserver
  2. start the truncation process by calling at least 1 setState. More setState + componentDidUpdate could be triggered in order to find the max. content that fits to the container.
  3. idle - go back to point 1. with a new update

As sidenote, the component handles a case when receiving new props and at the same time the truncation (phase 2) is in progress. Initiating truncation when the previous one hasn't finished could lead to incorrect result. This make me think about questions where I don't really know the answer:

  • can component receive new props while in updating process (setState called in componentDidUpdate) ?
  • is this connected to React async rendering ?
  • or maybe it could happen just in case the series of setStates happen in more than 1 browser reflow 🤷‍♂️

So IMO, the problem is that we need to distinguish what is the internal setState (for truncation, phase 2) vs external update (phase 1). UNSAFE_componentWillReceiveProps solves it quite well for this purpose. Earlier I thought that getDerivedStateFromProps can, with some limitations, do the same job - that is not the truth. Since [email protected] getDerivedStateFromProps is triggered for all updates including those from setState - see http://projects.wojtekmaj.pl/react-lifecycle-methods-diagram/ THERE IS NO EASY WAY TO FIGURE OUT UPDATE IS GOING TO HAPPEN BECAUSE OF PROPS CHANGE

The only viable solution I could think of is using new key or the one mentioned in the Alternative 2

For example the library could internally have 2 components

class WrapperThatRerendersWithNewProps extends React.PureComponent {
    render() {
      return <TruncateMarkup 
       {...this.props}
       key='new-key-for-every-rerender-that-effectively-remounts-TruncateMarkup'     
      />
  }
}

Any be better ideas? There has to be something 😬

@patrik-piskay
Copy link
Owner

Yes, I believe having the library consumers to set key to remount is the best option we have.

I don't think the "updating with new props" case is something we need to support, mainly because I'm not aware of use cases where updating with new props is necessary, except maybe when fetching the data from server. But that should be handled by conditional rendering (rendering RTM only once the data is ready).

For other use cases where the text changes dynamically, it's probably reasonable to expect a new key provided by users.

Regarding your proposed WrapperThatRerendersWithNewProps type of a component, I think that should also be the kind of abstraction that should be handled by users, if that's what they want to do.

@mundanelunacy
Copy link

IMHO, best solution would be to refactor to a functional component using useState and useEffect hooks

@ramosbugs
Copy link

React Strict Mode complains about this now:

Warning: Using UNSAFE_componentWillReceiveProps in strict mode is not recommended and may indicate bugs in your code. See https://reactjs.org/link/unsafe-component-lifecycles for details.

* Move data fetching code or side effects to componentDidUpdate.
* If you're updating state whenever props change, refactor your code to use memoization techniques or move it to static getDerivedStateFromProps. Learn more at: https://reactjs.org/link/derived-state

Please update the following components: TruncateMarkup

If I'm reading the docs right, it looks like the main consequence is that the future React concurrent rendering feature won't work with unsafe lifecycles.

@bonesoul
Copy link

i'm also getting the same warning @ramosbugs , any solutions?

@elishaterada
Copy link

Any plan to address UNSAFE_componentWillReceiveProps issue? If someone in the community converts the class component to functional, would PR be reviewed and accepted? Or are we better off forking and releasing it under the new npm name? I use this package often so I might take time to help with this.

@patrik-piskay
Copy link
Owner

patrik-piskay commented May 27, 2022 via email

@stropho
Copy link
Author

stropho commented May 27, 2022

I am not the maintainer but I've contributed to this repo a few times. IMHO if the new functional component works the same - accepting same props, displays the same result and does not introduce performance issues, I don't see why it shouldn't be merged.
(EDIT: Hehe, and the maintainer replied faster than me :) )

@bonesoul
Copy link

any changes we'll get a fix soon?

@hc0503
Copy link

hc0503 commented Mar 22, 2023

this issue is yet happened.

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

No branches or pull requests

8 participants