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

Discussion: componentWillReceiveProps vs getDerivedStateFromProps #721

Closed
arieh opened this Issue Mar 28, 2018 · 90 comments

Comments

Projects
None yet
@arieh

arieh commented Mar 28, 2018

Hello

We have found a scenario where the removal of componentWillReceiveProps will encourage us to write worse code than we currently do.

We currently consider props to be a valid form of input and state for our component. If a component received a prop, we do not duplicate it into our state as to maintain a single source of truth. This is especially true when using state management libraries where the state is externalized from the component but will also be true for many other scenarios.

However, with the removal of componentWillReceiveProps, react will force us to use setState for any prop we would like to monitor for change. Basically, any type of prop change that would also trigger a following action (be it internal one or an external one) would require us to duplicate the prop into the state.

To give a sort of clean example, let's imagine a dialog that has a visible flag and would like to report when it was hidden to a logging framework. Currently, we can do:

componentWillReceiveProps(newProps){
      if (this.props.visible === true && newProps.visible === false) {
           registerLog('dialog is hidden'); 
      }
}

With the new paradigm, I will need to do:

static getDerivedStateFromProps(nextProps, prevState){
        if (this.state.visible === true && newProps.visible === false) {
           registerLog('dialog is hidden'); 
       }
        return {
               visible : nextProps.visible
        };
}

Aside from already duplicating the props, in my opinion this will heavily encourage users to do

static getDerivedStateFromProps(nextProps, prevState){
        return {
           ...prevState,
           ...props
        };
}

Since it takes away a lot of the usefulness of props.

@alexkrolick

This comment has been minimized.

Collaborator

alexkrolick commented Mar 28, 2018

This issue should probably be filed on the React repo or the RFC repo rather than docs, but... 🤥nvm

  1. You can still use props in render, no need to copy into state unless you are computing a new state using new props and current state (e.g. resetting values after a page transition or showing a confirmation dialog).
  2. You can return null from gDSFP if you want to perform side-effects based on prop changes without doing anything to state (your example could do this)
  3. If you need access to the component instance (this), you should be able to use componentDidUpdate to perform side-effects (note: after the commit/render phase)
@SimenB

This comment has been minimized.

SimenB commented Mar 28, 2018

Came to report the same thing. In our case, we use https://github.com/final-form/react-final-form, and wanted to clear the value of one field when another field changed (think 2 dropdowns, one with car manufacturer and one with model - when manufacturer changes, clear model). Then we had to compare old props with new props without putting it in state, same as OP.

We've since migrated to use https://github.com/final-form/final-form-calculate which allows us to do the same thing declaratively, but I still think it's a valid use case.

(I guess the answer, if we had not moved away from it, would have been "lift state up". Which is fair enough 🙂)

This issue should probably be filed on the React repo or the RFC repo rather than docs

The blog post encouraged opening issues here: https://reactjs.org/blog/2018/03/27/update-on-async-rendering.html#other-scenarios

@alexkrolick

This comment has been minimized.

Collaborator

alexkrolick commented Mar 28, 2018

Alright, let's leave it open then

@alexkrolick alexkrolick reopened this Mar 28, 2018

@alexkrolick alexkrolick changed the title from Removal of componentWillReceiveProps will encourage duplication of state/data to Discussion: componentWillReceiveProps vs getDerivedStateFromProps Mar 28, 2018

@timroes

This comment has been minimized.

timroes commented Mar 28, 2018

I added another use-case into (the duplicate) issue #729

@gaearon

This comment has been minimized.

Member

gaearon commented Mar 28, 2018

Can you explain why componentDidUpdate doesn't work for this?

Before:

componentWillReceiveProps(newProps){
      if (this.props.visible === true && newProps.visible === false) {
           registerLog('dialog is hidden'); 
      }
}

After:

componentDidUpdate(prevProps){
      if (prevProps.visible === true && this.props.visible === false) {
           registerLog('dialog is hidden'); 
      }
}

That’s the exact use case for componentDidUpdate and it’s more accurate than componentWillReceiveProps because it only fires when the prop has actually changed.

@danburzo

This comment has been minimized.

Contributor

danburzo commented Mar 28, 2018

I think in some cases moving a prop to state is the right solution.

Assume you need to perform an impossibly expensive computation based on a prop, and you want to make sure you only perform it when the prop has actually changed. And supposedly you need the computed value for rendering.

In this case, instead of doing:

class Component extends React.Component {

  constructor(props) {
    super(props);

    this.state = {
      computed_prop: heavy_computation(props.value)
    };
  }

  componentWillReceiveProps(newProps) {
    if (newProps.value !== this.props.value) {
      this.setState({
        computed_prop: heavy_computation(new_props.value);
      })
    }
  }
}

we can instead:

class Component extends React.Component {
  static getDerivedStateFromProps(props, current_state) {
    if (current_state.value !== props.value) {
      return {
        value: props.value,
        computed_prop: heavy_computation(props.value)
      }
    }
  }
}

(Of course it would be nice not to store this.state.value based on this.props.value — so I do wonder if there is another way to do this more elegantly)

@arieh

This comment has been minimized.

arieh commented Mar 28, 2018

@gaearon - I do not see a great difference other than a mind shift I guess. I would suggest in this case to add this transition to the docs maybe.

@arieh

This comment has been minimized.

arieh commented Mar 28, 2018

Since I see the title changed on this topic I'll add another use case that becomes harder (haven't interacted with react issues before so please let me know if I should open a new ticket instead :) )-

We currently use willReceive to do various pieces of logic in preparation for render (input parsing, sorting etc). Although technically getDerivedStateFromProps can be used for this, one issue that arrises is that if previously we could use class methods to organize our code, we now have to either have a lot of code in our getDerived or to use non-method functions for it. This feels like it's forcing us to take code that used to belong to the class now have to stay outside it and then code around these limitations (pass state around as arguments to functions, avoid using this/wrap/bind etc) due to the static nature of the new function.

@gaearon

This comment has been minimized.

Member

gaearon commented Mar 28, 2018

I don’t have a great answer for you here—indeed class methods like this tend to have code that’s unsafe for async which is why we want it to be static. So yes, you’d need to pull that out if you want the code to be async-friendly. One upside of doing this is that it should be easier to test because it’s separated from the rest of the class logic.

@mattkrick

This comment has been minimized.

mattkrick commented Mar 29, 2018

My primary use case is similar to @danburzo's where I need to compute state derived from a prop on initial build & whenever it changes (for example, filtering a list). The 2 solutions I arrived at are:

class ListSomeThingsV1 extends Component {
  static getNextState = (allTheThings) => {
    return {
      allTheThings,
      someThings: allTheThings.filter((thing) => !thing.hide);
    }
  };
  static getDerivedStateFromProps(nextProps, prevState) {
    if (nextProps.allTheThings !== prevState.allTheThings) {
      return ListSomeThingsV1.getNextState(nextProps.allTheThings);
    }
    return null;
  }
  state = ListSomeThingsV1.getNextState(this.props.allTheThings);
}

class ListSomeThingsV2 extends Component {
  state = this.getNextState();
  getNextState() {
    const {allTheThings} = this.props;
    return {
      someThings: allTheThings.filter((thing) => !thing.hide)
    }
  }
  componentDidUpdate(prevProps) {
    if (prevProps.allTheThings !== this.props.allTheThings) {
      this.setState(this.getNextState())
    }
  }
}

Which way is preferred? Is there a cleaner alternative to both? The first isn't great because I need to keep the original object in my state just for an equality check & I need the 2nd static function to stay DRY. The 2nd just feels strange because I'm setting state in cDU so it could cause an extra render (please correct me if I'm wrong!).

@gaearon

This comment has been minimized.

Member

gaearon commented Mar 29, 2018

@mattkrick

The second way is definitely worse because you're doing an extra update and because you get an intermediate render where state is inconsistent with props.

The first isn't great because I need to keep the original object in my state just for an equality check

This is intentional. Forcing you to keep it in the state explicitly lets React not hold onto the previous props object in the majority of use cases in the future. So this is better from memory usage perspective.

Regarding duplication, you could write it like this to avoid the duplication:

class ListSomeThingsV1 extends Component {
  static getDerivedStateFromProps(props, state) {
    if (props.allTheThings !== state.prevAllTheThings) {
      return {
        prevAllTheThings: props.allTheThings,
        someThings: props.allTheThings.filter((thing) => !thing.hide);
      }
    }
    return null;
  }
  state = {
    prevAllTheThings: [],
    someThings: [],
  };
}

Those initial values will be overridden anyway. Don’t forget getDerivedStateFromProps gets called for before the first render too.

@iulian-r

This comment has been minimized.

iulian-r commented Mar 30, 2018

I think an use case missing here is the following:

  • Our component is connected to a Redux store via connect() and has some extra props for itself.
  • We use the store to save in it a form which is used as a base for requesting data, data displayed by our component.
  • Any change in the form trigger an action which changes the store. The connect() triggers a new set of props for our component. We want to first fetch the new data and only then render it.
  • To block the rendering when only the props coming from store are changed we implement shouldComponentUpdate which implies there is no componentDidUpdate called so we cannot make an API call from it. It looks like we need to make the call from shouldComponentUpdate. Is it that OK, as it looks like a side-effect for me calling the API from there?
@gaearon

This comment has been minimized.

Member

gaearon commented Mar 30, 2018

To block the rendering when only the props coming from store are changed we implement shouldComponentUpdate

That sounds like the main issue here. shouldComponentUpdate is intended to be more like a hint to React that it could skip an update than an instruction to block rendering. We’re working on a feature that lets you do this kind of blocking, but in the meantime, the React-friendly solution would be to maintain a copy of the props you want to “keep constant” in the state (via getDerivedStateFromProps) and use that as a source of truth. Then reset them to new values when all new data is ready. The rule of thumb is that the app shouldn’t look differently if shouldComponentUpdate was turned off. It’s an optimization, not a data flow mechanism.

@iulian-r

This comment has been minimized.

iulian-r commented Apr 4, 2018

Sorry, I was not that clear.
The problem is that now we call the API from componentWillReceiveProps as changes of the props (coming from store in our case) requires fetching of new data. As this function is deprecated, we will need to do this in a different place. One of it can be shouldComponentUpdate. An other can be getDerivedStateFromProps. But doing the call there is feeling like abusing these functions for doing something different then it is supposed. We can do the call from componentDidUpdate, but if we want to avoid unneeded renders we are forced to make shouldComponentUpdate returns false. And this make componentDidUpdate not being call at all.

@gaearon

This comment has been minimized.

Member

gaearon commented Apr 4, 2018

You can split your component in two. The outer one does the data fetching in componentDidUpdate and just renders the inner one. The inner one has shouldComponentUpdate. This should be enough to solve your problem.

@iulian-r

This comment has been minimized.

iulian-r commented Apr 4, 2018

Great to see that your approach is what I would like to have too :) Thanks Dan.

@gaearon

This comment has been minimized.

Member

gaearon commented Apr 4, 2018

Yeah. It’s not the most elegant solution but it’s also not a very common case because usually people need to re-render on prop change. If you intentionally want to wait for data to be fetched, longer term Suspense API will be a natural solution to that. But before you can use Suspense you’ll need components to be async-safe 🙂.

@iamakulov

This comment has been minimized.

iamakulov commented Apr 4, 2018

@danburzo

(Of course it would be nice not to store this.state.value based on this.props.value — so I do wonder if there is another way to do this more elegantly)

I’d make the function memoized (i.e., add a cache to it) and just call it in render() each time. To me, this seems a cleaner solution (because the component doesn’t need to care if the function is heavy or not):

const heavyComputation = _.memoize((value) => { ... });

const Component extends React.Component {
  render() {
    // Takes a while for the first call, just returns a cached result for next calls
    const computedProp = heavyComputation(this.props.value);
    return <div>{computedProp}</div>;
  }
}
@gaearon

This comment has been minimized.

Member

gaearon commented Apr 4, 2018

Note you'd want to either scope memoization to a component instance or use some kind of LRU cache.
Otherwise if you have a list of items with different inputs they will keep invalidating each other.

@manuelbieh

This comment has been minimized.

manuelbieh commented Apr 4, 2018

Not 100% on topic but close to: toying around with the new lifecycle methods yesterday made me realize that it might in some cases force developers to break encapsulation of components since you have to "extract" helper functions which were, for the sake of convenience, class instance methods before and rewrite the way they work to a certain extent.

Made up example, most simple use case I could think of:
https://codesandbox.io/s/14k7nw0rp4

componentDidUpdate would be too late here, getDerivedStateFromProps on the other hand, would not be able to access the this.setValue class method.

I'd probably have to make that if (value === 'cheesecake') part and this.sanitizeValue a function that lives outside of and independent from the component so I don't have to duplicate too much of the logic inside of getDerivedStateFromProps.

While I can understand the reasons behind making getDerivedStateFromProps static, I'm not really sure if it is really a good idea to do so.

@gaearon

This comment has been minimized.

Member

gaearon commented Apr 4, 2018

I think the notion that a method is more “encapsulated” than a function in the module scope is mistaken. If anything, a function declared in the module scope is more encapsulated because it’s not accessible by external code at all. Whereas a class method declared on a prototype (or even on the instance) is accessible by any external code that has an instance handle.

If you insist on declaring all functions on the class (as opposed to putting them next to it in the same module) you can declare them as static. You can call static methods from instance methods so that lets you reuse the code. Although I’d argue that declaring such helpers outside is not just convenient but better from the encapsulation perspective.

@theKashey

This comment has been minimized.

theKashey commented Apr 4, 2018

The main concern - sometimes you have to call javascript function, in response to props change. Usually it should not matter where/when you do it - before render, or after.

Sometimes it could cause double renders -> render component cos prop changes and you have to render it to trigger didUpdate -> send an update to a thirdparty (the first time you have an instance) -> get response -> render everything again -> didUpdate again -> think how to prevent loop.

Splitting component could help, but this is "more boilerplate code", yet again. This could be a mess.

PS: A month ago I've spiked getDerivedStateFromProps+memoization, working perfectly in react-memoize and 16.3 redux spike(non official)

@manuelbieh

This comment has been minimized.

manuelbieh commented Apr 4, 2018

Valid point. I will try to come up with a more realistic example when I'm home later today. Let's see. Maybe I only have to get used to that new getDerivedStateToProps thing as many of my components in the past relied on componentsWillReceiveProps calling methods inside of the components they were defined in 😉

@manuelbieh

This comment has been minimized.

manuelbieh commented Apr 4, 2018

Okay. After having written half of a novel I realized: everything I described could very likely be rewritten to use componentDidUpdate instead of componenWillReceiveProps and it will most certainly never be too late.

Found in general only 2-3 places where componentWillReceiveProps was not only used to setState based on future props. One of these cases would have even been better off with componentDidUpdate instead because there's also a shouldComponentUpdate check in place, which would save one unnecessary this.hasCustomerDataChanged(this.props, nextProps) in cWRP if cDU were used instead.

So nope, all fine 😉👍

@danielrob

This comment has been minimized.

danielrob commented Jun 10, 2018

I was coming back to delete my comment because I realized indeed that was the trivial solution. Not sure how I got confused there (translating from real world apps to sandbox cases and back again, perhaps). Anyway, that case of what-not-to-do is covered now as well. Thanks Dan.

@francisrod01

This comment has been minimized.

francisrod01 commented Jun 20, 2018

I don't see the way to do this without componentWillReceiveProps():

Live examples:

  1. Using old React lifecycle: https://snack.expo.io/r1bMAvN7z
  2. Trying to use new React lifecycle: https://snack.expo.io/HkmQCPUb7

Below you'll see a little bit about the react-native-calendars marked items implementation:

const parseMarkedDates = (markedDates, bookingDays) => {
    const parseDates = {}
    Object.keys(bookingDays)
        .map(kDate => parseDates[kDate] = bookingDays[kDate].calendar)
    
    // Create a new object using object property spread since it should be immutable.
    // Reading: https://davidwalsh.name/merge-objects
    const newMarkedDatesObject = {
        ...markedDates,
        ...parseDates,
    }

    return newMarkedDatesObject
}

The Calendar screen:

class ReservationServiceScreen extends Component {
    constructor(props) {
        super(props)

        // Defining consts.
        const { navigate } = props.navigation
        this.navigate = navigate

        const { service } = props.reservCalendarSrv
        const { bookingDays } = props.serviceDetails

        const periodTimes = [
            { value: '4h', 'label': '4 hours (half a day)' },
            { value: '8h', 'label': '8 hours (all day)' },
        ]
        const _format = parseTimes.dateFormatString
        const _today = new Date().toISOString().substring(0, 10)
        const _maxDays = 15

        const markedDates = {}
        const selectedDays = {}


        // Defining states.
        this.state = {
            service,
            _format,
            _today,
            _maxDays,
            bookingDays,
            periodTimes,
            markedDates,
            selectedDays,
        }

    componentDidMount() {
        // Load disabledDates just one time.
        if (!this.hasMarkedDates() || !this.hasBookingDays()) {
            this.generateDisabledDates()
        }

        if (this.hasMarkedDates() && this.hasBookingDays()) {
            const { markedDates, bookingDays } = this.state

            const parsedMarkedDates = parseMarkedDates(markedDates, bookingDays)
            this.updateMarkedDatesState(parsedMarkedDates)
        }
    }

    componentWillReceiveProps(props) {
        if (this.hasBookingDays()) {
            const { bookingDays } = props.serviceDetails
            this.updateBookingDaysState(bookingDays)

            const { markedDates } = this.state
            const parsedMarkedDates = parseMarkedDates(markedDates, bookingDays)
            this.updateMarkedDatesState(parsedMarkedDates)
        }
    }

    hasMarkedDates() {
        const markedDates = this.state.markedDates
        return !!Object.keys(markedDates).length
    }

    updateMarkedDatesState(markedDates) {
        this.setState({ markedDates })
    }
    updateBookingDaysState(bookingDays) {
        this.setState({ bookingDays })
    }

The componentWillReceiveProps(props) implementation is about bookingDays object to become another screens as well.

More about the react-native-calendars implementation:

(This running using old React lifecycle): wix/react-native-calendars#283 (comment)

@danielrob

This comment has been minimized.

danielrob commented Jun 20, 2018

@francisrod01 Everything happening in your componentWillReceiveProps (based on the methods you've shown) is involved only with reading and setting new state. getDerivedStateFromProps would be a drop in replacement for this case. As mentioned though, you'll need to use pure functions instead of instance functions.

   const hasBookingDays = state => state.bookingDays // extract state 'selector' to pure function if needed

   static getDerivedStateFromProps(props, state) {
        if (hasBookingDays(state)) { 
            const { bookingDays } = props.serviceDetails
            return {
               ...state, 
               bookingDays,
               markedDates: parseMarkedDates(state.markedDates, bookingDays),
            }
        }
    }

To avoid recomputation you might want to memoize this.

@francisrod01

This comment has been minimized.

francisrod01 commented Jun 22, 2018

Thank's, @danielrob !! I liked this solution!! It covered must all the scenario.
Using this implementation, I'd like to know how I can to call to this.generateDisabledDates() to show the disabled dates after I navigate between screens and I return to this screen.

I mean, using this implementation, I lost the disabled dates because this method is called only in componentDidMount and in the old componentWillReceiveProps(..).

@DeepaSriramRR

This comment has been minimized.

DeepaSriramRR commented Jul 6, 2018

Hi, I have the following use-case. On a push notification, I need to refresh a DrawerNavigator view. When the user clicks the notification, I use redux and store the notification type (notificationType).
In the view to be refreshed, I use redux connect and componentWillReceiveProps, and refresh the view by fetching modified data. componentWillReceiveProps will be invoked because the notification type changed in redux.

How can I replace this by componentDidUpdate. My problem is I dont want an infinite cycle in componentDidUpdate. How do I ensure this? I cant check for props changed from previousProps. notificationType will be set by redux and will be the same value. It won't change because of the fetchData() call.

@bvaughn

This comment has been minimized.

Contributor

bvaughn commented Jul 6, 2018

I don't quite understand what you're saying, @DeepaSriramRR.

componentWillReceiveProps will be invoked because the notification type changed in redux.

If componentWillReceiveProps is called in your first case because of a notificationType props change, then why wouldn't that be detectable in componentDidUpdate (by comparing this.props.notificationType to prevProps.notificationType)?

If there's an async fetching state involved in your component, then you could store the status of that in state and compare this.state.whatever to prevState.whatever instead.

@DeepaSriramRR

This comment has been minimized.

DeepaSriramRR commented Jul 6, 2018

In componentDidUpdate, this.props.notificationType will be equal to prevProps.notificationType, because this.props.notificationType is from redux, and the value doesn't change after a call to fetchData which changes just state:{data}.

I need a way to ensure that only the first call to componentDidUpdate after this.props.notificationType is set calls fetchData. When fetchData changes the state:{data}, as I understand componentDidUpdate will be called again. How do I ensure fetchData is not called again. I cant do it based on this.props.notificationType. The value will be the same as in the previous call.

fetchData updates an order's details and refreshes it, if there are any changes. If there are no changes to the order details, the data will be unchanged. So I cant rely on this.state.orderDetails changing in fetchData.

This is my current code which works properly using componentWillReceiveProps.

{
constructor(props) {
  super(props);
  this.state = {
    orderReceipts: [],
  };
  this.fetchData = this.fetchData.bind(this);
}

fetchData() {
  return api.getOrderReceipts(this.props.token, this.props.customer.id)
  .then((orderDetails) => {
    this.setState({orderReceipts: orderDetails.data, loading: false});
  }).catch((error) => {
    this.setState({status: error, loading: false});
  });
}

componentWillReceiveProps() {
  console.log("this.props.notificationData = " + JSON.stringify(this.props.notificationData));
  this.fetchData();
}
}

function mapStateToProps({notificationData}) {
return {
  notificationData
}
}


@DeepaSriramRR

This comment has been minimized.

DeepaSriramRR commented Jul 6, 2018

Just to add to the comment, I am trying to refresh order details when a user clicks a push notification. I decided to do it by changing the props in redux on receipt of a notification, so that componentWillReceiveProps will be invoked and I can refresh it. Note that a subsequent push notification should also refresh the order page, even though the notificationData type can be the same.

@bvaughn

This comment has been minimized.

Contributor

bvaughn commented Jul 6, 2018

I think I understand what you're asking now.

In componentDidUpdate, this.props.notificationType will be equal to prevProps.notificationType, because this.props.notificationType is from redux, and the value doesn't change after a call to fetchData which changes just state:{data}.

I need a way to ensure that only the first call to componentDidUpdate after this.props.notificationType is set calls fetchData. When fetchData changes the state:{data}, as I understand componentDidUpdate will be called again. How do I ensure fetchData is not called again. I cant do it based on this.props.notificationType. The value will be the same as in the previous call.

In the scenario you're describing, componentDidUpdate will be called twice.

The first time, after Redux re-renders your component with a new notificationType prop, this.props.notificationType and prevProps.notificationType will be different, letting you know to start the fetch.

The second time, after your fetch completes and updates state.data, this.props.notificationType and prevProps.notificationType will be the same, so you don't have to do anything.

The prevProps and prevState parameters passed to componentDidUpdate are the props and state that the component was last rendered+committed with. In a case where a re-render occurs because of a change in state only (no new props) then the values inside of prevProps and this.props will be the same.

@DeepaSriramRR

This comment has been minimized.

DeepaSriramRR commented Jul 7, 2018

@bvaughn Thank you for the reply.
I solved this by adding a time to the notification and doing a setState{updated: this.props.notificationData.notificationTime} in fetchData. Now I check the following:

componentDidUpdate(prevProps) {
  if (this.props.notificationData.notificationType != null && this.state.updated != this.props.notificationData.notificationTime) {
      this.fetchData();
    }
}

I currently have this.fetchData() in componentDidMount as well because I want the data to be refreshed when I navigate to the page. Will this cause problems with fetchData in both componentDidUpdate and componentDidMount ?

@bvaughn

This comment has been minimized.

Contributor

bvaughn commented Jul 7, 2018

That usage of state doesn't seem necessary. (Have you read this blog post, by chance?)

I don't think you've really explained yet why this isn't sufficient:

componentDidUpdate(prevProps) {
  if (this.props.notificationData.notificationType !== prevProps.notificationData.notificationType) {
      this.fetchData();
    }
}

I currently have this.fetchData() in componentDidMount as well because I want the data to be refreshed when I navigate to the page. Will this cause problems with fetchData in both componentDidUpdate and componentDidMount ?

No, so long as the fetch in componentDidUpdate is behind a conditional check like the one I wrote above.

@DeepaSriramRR

This comment has been minimized.

DeepaSriramRR commented Jul 8, 2018

Hi @bvaughn, the first time notificationType changed because of a new push notification, the check this.props.notificationData.notificationType !== prevProps.notificationData.notificationType will work. This is when notificationType was set to "orderReady" for example.

When there is another push notification sent to the user, (and all subsequent push notifications from here) prevProps.notificationData.notificationType will be "orderReady", and this.props.notificationData.notificationType will also be equal to "orderReady".
Because its the same push notification type, but the page has to be refreshed by fetching new data.

This is why I did the state based check. Hope it's clear.

@bvaughn

This comment has been minimized.

Contributor

bvaughn commented Jul 8, 2018

Hmm, I think I see, although I'm not sure how componentWillReceiveProps would have worked for you either in this case. 😄

Could you compare this.props.notificationData !== prevProps.notificationData? Does this wrapper object only get created/recreated when there's a new notification?

@DeepaSriramRR

This comment has been minimized.

DeepaSriramRR commented Jul 9, 2018

Yes I can compare the notificationData.notificationTime now. Thats why I added notificationTime to redux now. notificationData gets created/recreated only when there's a new notification. And I can check noticationTime having changed by checking props and prevProps.

Thank you @bvaughn

@tischsoic

This comment has been minimized.

tischsoic commented Jul 14, 2018

@DeepaSriramRR As far as I understand, you want to trigger fetch whenever a user clicks the notification, even if this.props.notificationData stay the same. Currently, when a user clicks the notification, you update the redux store which in turn provides new props for your component. The problem is that you try to describe action using data (user's click -> props). In my opinion, this is the root cause of your problem. Action should be described by a function call in this case. You can call, let's name it refreshData(), on your component after a user clicks on the notification, which would result in calling fetch.
Or better, that data can be fetched in some async redux action.

Moreover, considering your new approach,
you store notificationTime within the component state. I am not sure if it is the best place for that data. Rendering of the component does not directly depend on notificationTime and I think that better place for notificationTime in your component is a private property of the component's class. Then, in componentDidUpdate you would use < to compare this.updated < this.props.notificationData.notificationTime.

What do you think, @bvaughn ? I wonder if my idea is good. :-)

@DeepaSriramRR

This comment has been minimized.

DeepaSriramRR commented Jul 18, 2018

@tischsoic Thank you for the response. I use react-navigation to navigate to a screen in response to the notification handler. Its a DrawerNavigator so doesn't take props.
However there is no way I could think of, to call refreshData on the component in response to the notification. Thats why I used redux to change data and respond to the change by refreshing data. If there's a way I can invoke refreshData directly on the component it would be the best option for me.

You are right about notificationTime not needed to be in state. I moved it to be a private property of the component.

@flygis

This comment has been minimized.

flygis commented Aug 3, 2018

Hi guys, I have a new question related to this issue. I have started to migrate away from componentWillReceiveProps in a commercial project I am developing, and everything else seems to be going just great, but I have no idea how to solve a following type of situation (which there are a lot throughout the project).

I have a React component, which is connected into a redux store (so a container component), and in this component I handle all the data changes etc. and re-render child components based on those changes.

So my problem is like the following, I have a list of lets say buttons, and the buttons have certain data passed them as props, the data might change from time to time, and based on the changes I re-render them, I used to handle a lot of these kinds of re-creations of child components in componentWillReceiveProps, and now when I am changing it into getDerivedStateFromProps, I cannot bind my parent components functions into them anymore. So what would be a correct way of doing this kind of thing with the new static method (or some other React 16.4+ class method)

              var usersButtons = [];
			  for (var i = 0; i < nextProps.buttons.length; i++) {
                  usersButtons.push(
                    <PunchButton
                      buttonText={nextProps.buttons[i].buttonText}
                      handleButtonClick={this.handleButtonClick}
                      icon={icon}
                      id={nextProps.buttons[i].id}
                      key={nextProps.buttons[i].id}
                      returnTimeEnabled={nextProps.buttons[i].returnTimeEnabled} />
                  );
             }

Should I just do this mapping in different method or what? I have tried to read comments in this post and blog posts, but I am still not fully comfortable of what to do.

Can anyone help me with this?

@benadamstyles

This comment has been minimized.

benadamstyles commented Aug 3, 2018

@flygis Is there a reason you're pushing elements into an array and not just putting them directly in your container component's render method? Something like:

render() {
  return (
    <div>
      {
        this.props.buttons.map(button => (
          <PunchButton
             buttonText={button.buttonText}
             handleButtonClick={this.handleButtonClick}
             icon={icon}
             id={button.id}
             key={button.id}
             returnTimeEnabled={button.returnTimeEnabled} />
        ))
      }
    </div>
  )
}
@bvaughn

This comment has been minimized.

Contributor

bvaughn commented Aug 3, 2018

It's not obvious to me why you need to use either componentWillReceiveProps or getDerivedStateFromProps in the situation you describe. Why not just render the buttons like @Leeds-eBooks mentions and let React handle them? Provided the button.id is stable, React will re-use button instances when re-rendering.

@flygis

This comment has been minimized.

flygis commented Aug 6, 2018

@Leeds-eBooks @bvaughn That is definitely an option, but if I remember correctly, I once read that there shouldn't be that much conditional rendering in the render method. And I have a lot of checks I need to do before I can render some of these elements. As I said this was just one example.

But maybe I'll have to try to use the render method more to do the mapping, and somehow make the if statements separately in getDerivedStateFromProps or somewhere.

Little bit off topic, but is it actually heavy for react to do some of these if else statements in the render method before the return? That could be a solution for me since I can access both the components internal state and the props there.. Just thinking out loud here.

@benadamstyles

This comment has been minimized.

benadamstyles commented Aug 6, 2018

@flygis I'm sure @bvaughn will be able to correct me if I'm wrong, but the render method is exactly where you're expected to put conditional rendering. Don't be scared to question "advice" you read – including mine! – because there's a lot of stuff written about React which is overbearing, too concrete, or just plain wrong. If in doubt, check the React documentation (e.g. on conditional rendering).

The fundamental point of the render method is to output UI based on state and props. If essentially what you're doing is checking conditions based on state and props, and deciding what to render – that is exactly what React is for and render is one of the main APIs of React!

If your render method is starting to feel too heavy, don't just add methods – make new components! Split your big render method into smaller components, and either keep them in the same file or move them out into separate files, whatever works for you. If you're using React, you've already made the (excellent) decision to fully embrace components, so don't be frightened of them. As a rule of thumb, the more you use components over methods, the more you're playing the React game how it is supposed to be played and the more React will reward you with understandable, robust code that's easy to reason about.

@flygis

This comment has been minimized.

flygis commented Aug 6, 2018

@Leeds-eBooks I actually found that page you mentioned before you added the comment, and started to move the conditional rendering into return function, by that I managed to almost completely get rid of the getDerivedStateFromProps. So thank you for the help!

@bvaughn

This comment has been minimized.

Contributor

bvaughn commented Aug 6, 2018

and started to move the conditional rendering into return function, by that I managed to almost completely get rid of the getDerivedStateFromProps

That's good to hear! 😄

@Leeds-eBooks's advice sounds solid. Conditional rendering is fine, and breaking up complex logic into separate React components can also be a good option in some cases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment