This pull request is automatically deployed with Now.
To access deployments, click Details below or on the icon next to each push.

Latest deployment for this branch: https://websiteone-fe-git-fork-jmpainter-165-replace-componentw-4c766c.agileventures1.now.sh

hey @jmpainter, sorry I haven't responded to you earlier, I saw this, but had a short day yesterday and was trying to get another PR ready for a different project.

We have an issue to do this, which you have linked to so I guess you have seen. Thanks for getting this going for us! I guess your doubt is about using componentDidUpdate as an alternative?

I think most of the cases we are using componentWillReceiveProps because the props are udpated after an axios request resolves from the backend, is that what you saw?

In that case, using componentDidUpdate seems like a safer alternative.
Have you tested out the changes you made in the browser? Does it all work as expected?
I can pull it down later and have a look, now I'll try to make some progress on finishing off that hangout sharing ticket you put in... #backendfun

Hi, no problem - I figured this was not a pressing issue but it is an interesting one so I thought I'd work on it. What I'm seeing is that in the case of ProjectList and UserList, componentDidUpdate can be used to replace what is currently in componentWillReceiveProps. These cases involve state changes that are done in response to a successful update from the api. In all the other cases, I think componentWillReceiveProps is not needed at all. The contained component can check for complete props from the parent's update. Everything looks ok with this solution locally. If this strategy looks ok, I can change the tests.

it looks ok to me, of course there is always the possibility someone else on the team will disagree, but I'd say if they haven't made their opinion known till now, maybe just go with it, and if @joaopapereira agrees as well, it seems like a definite improvement!

Ok, great - I'm going to fix the tests.

I'm not sure if this is a compelling reason @Sherspock, but I found this article
https://reactjs.org/blog/2018/06/07/you-probably-dont-need-derived-state.html
which suggests using getDerivedStateFromProps sparingly.
It doesn't say anything about componentDidUpdate though, so I'm not sure when it's appropriate to use one or the other, however I am trying to read more about it here https://stackoverflow.com/questions/49449527/why-use-getderivedstatefromprops-instead-of-componentdidupdate
and I'll follow the discussion.

@jmpainter I'll have to pull this down and interact with it, I think there might be code removed here that was specifically added to deal with edge cases you might not have run into when manually testing it, though I could easily be mistaken

I also had looked at the article 'You probably don't need derived state":
https://reactjs.org/blog/2018/06/07/you-probably-dont-need-derived-state.html
componentDidUpdate does cause an additional render, so maybe getDerivedStateFromProps would be a better choice in the two containers where it is used in this PR. The tests that were removed had to do with testing the setting of component state that has now been removed from some of the containers. This is the main idea of this PR. For example, EventInfo had local state for event. I'm not sure why this state is needed when the event is coming in as props through Redux. My assumption was so that EventSummary does not end up with an incomplete event prop before fetchEventInfo completes. My idea is that you can just make the check in EvenSummary and not worry about mirroring event in the component state and using something like componentWillReceiveProps. Maybe I am missing something about the need to store the data that is coming in through an api call in componentDidMount in component state.

I also had looked at the article 'You probably don't need derived state":
https://reactjs.org/blog/2018/06/07/you-probably-dont-need-derived-state.html
componentDidUpdate does cause an additional render, so maybe getDerivedStateFromProps would be a better choice in the two containers where it is used in this PR. The tests that were removed had to do with testing the setting of component state that has now been removed from some of the containers. This is the main idea of this PR. For example, EventInfo had local state for event. I'm not sure why this state is needed when the event is coming in as props through Redux. My assumption was so that EventSummary does not end up with an incomplete event prop before fetchEventInfo completes. My idea is that you can just make the check in EvenSummary and not worry about mirroring event in the component state and using something like componentWillReceiveProps. Maybe I am missing something about the need to store the data that is coming in through an api call in componentDidMount in component state.

@jmpainter @mattwr18
After reading the two articles mentioned above, I'm less concerned about the unneeded multiple render calls. I wouldn't push for a rewrite there unless this is creating user-facing issues, which I haven't checked and don't currently intend to check.

@mattwr18
I am mostly leaving this review to you -- including the not clearly motivated styling changes, which I personally think are a mistake. The only reason I jumped in was because the reason for switching from will-receive to did-update was not immediately apparent and seemed potentially problematic. The other observations were opportunistic.

@mattwr18
One thing I should have mentioned in my review though is that the DidUpdate methods themselves look dicey. Especially for ProjectsList. But I'm an interloper over here, so I'm trying not to get overly involved.

ProjectsList

componentDidUpdate () {
    if (this.props.projects.length > 0 && this.state.languages.length === 0) {
      this.paginateProjects(this.props.projects)
    }
  }

UsersList

componentDidUpdate () {
    if (this.props.users.length > 0 && this.state.usersList && this.state.usersList.length === 0) {
      this.normalizeUsers(this.props.users)
    }
  }

First of all, I don't love that we are keying off of both state and props in these methods. I don't see why we wouldn't be fully in the state world at this point.

Second of all, we want projects to only paginate if languages is empty? And same for users?

Third of all, what happened to the behavior where we don't paginate if the first project or user in the collection has error set to true? I see nothing explaining or motivating this change.

The fact that the build is green despite apparent second and third issues suggests that either:

1. I am missing some things
2. We are missing tests for these things
3. Some combination of both

I'd like to add my own review here. From my perspective, this new approach absolutely is worse than what was there before. It doesn't seem quite right that ComponentDidUpdate would be the place to watch for the new props coming in through redux after the api call in ComponentDidMount. Furthermore, there is a real risk in creating an infinite loop caused by the state changes called for in ComponentDidUpdate. That's what all the conditional checks are about.

This pr was an attempt to replace componentWillRecieveProps with something else (or nothing). However, I don't claim experience or knowledge of how to fix this issue. This approach looks like it works, but that doesn't mean it's great. I'm attempting to learn as a developer by taking on issues that will be a challenge and getting constructive feedback that will help me improve. @Sherspock , do you have any suggestions for changes or improvements?

@jmpainter That is a super badass attitude.

I have some thoughts on implementation but I don't claim to be a React/Redux expert or even journeyman, so they may not serve much use beyond a conversation starter.

With that caveat I'll post them this weekend. @mattwr18 Also very happy to hear your thoughts in reply

I do think it's good to mention that -- as I understand it -- this PR was a proactive and very welcome experimental attempt to replace componentWillReceiveProps hooks, which are now discouraged. This work would likely have been a good candidate for a draft PR, assuming those are available to us (I've yet to do or see one in a project I'm working on.)

As I fallibly understand it, given that this work was done without a motivating issue -- and was supported by the brave and potentially very fruitful "I dunno, let's see" approach -- we're not set greatly set back if we scrap it and go in a different direction.

I'm not yet saying let's do that, I'm just saying let's not be afraid to do that if warranted.

I will go ahead and ask the general question:
Would it be appropriate to put this logic earlier in the data flow and/or component lifecycle? (the answer would seem to be yes)

And the specific questions:
Would it be appropriate to put this logic in the corresponding reducers?
Would it be appropriate to put this logic in an earlier component lifecycle hook (perhaps also extracting the conditional branching into shouldComponentUpdate)?

I also intend to defer on this PR (barring major objections) to @mattwr18 and other WebsiteOne regulars like @joaopapereira

Thanks for the remarks. A draft PR would have been good for this. I hadn't heard of it before now but it sounds like a good way to present code for discussion without implying it should be merged at that point or ever. After giving the situation with these containers some thought, I think what is going on here is that redux store state is being duplicated in local state or normalized and stored in component state. While it is not necessary to have all state in redux, It may be an anti-pattern to have a combination of redux store and local state for the same data or data that is derived from that data. From this article https://reactjs.org/blog/2018/06/07/you-probably-dont-need-derived-state.html

Common Bugs When Using Derived State:
"The most common mistake with derived state is mixing these two; when a derived state value is also updated by setState calls, there isn’t a single source of truth for the data."

What is going on in the ProjectsList.js and UsersList.js containers in componentWillReceiveProps amounts to data normalization that could be done as part of the redux flow through actions and reducers and stored in the redux store. Then redux could do its job and provide this to the containers as props. In the case of a container like EventInfo.js, the local state copy of what is in the redux store can be eliminated because it will be provided correctly by redux once the api call returns. Then componentWillReceiveProps can be eliminated without introducing issues with replacing it with componentDidUpdate.

My fallible thoughts on the subject:

Seems to me like what is warranted is extracting the useful bits of this PR that do not really relate to removing componentWillReceiveProps and putting them into another PR. This one got fairly scope creepy, and there's some useful stuff in here that doesn't appear to have much to do with the core work.

I would then close this PR. Personally, if I were a regular on this project, I would also insist that the fat empty object literal change not be part of this or any subsequent PR without reasonable justification.

Some more thoughts, understanding that I will defer to decisions made by the team in this neck of the woods:

I question the general approach we're using for both pagination and state management for the ProjectsList and UsersList components. It seems odd to me that we're home-rolling pagination for these components, that the logic hasn't cohered around a concept, and the names of those concepts are not consistent -- why is essentially the same low-level munging called normalize for Users but paginate for projects?

It also seems odd that the components seem to only ever care about paginated data to display, yet we're letting them get initialized with data that isn't paginated. If they really only care about paginated data, why wouldn't we move the pagination logic into the corresponding reducers?

I suspect we could get rid of a lot of needless state management and wondering where things should go by off-loading the pagination responsibility completely and hooking into a pagination library.

Things seem tangly here, but I think that if we're going to take the time to bring these components into compliance with preferred lifecycle methods, we should also take the time to think about and discuss the right way to do it.

An alternative would be to perform this conversion in a more disciplined way and ensure that the logic in the new lifecycle methods is functionally identical from a user's perspective.

What is going on in the ProjectsList.js and UsersList.js containers in componentWillReceiveProps amounts to data normalization that could be done as part of the redux flow through actions and reducers and stored in the redux store. Then redux could do its job and provide this to the containers as props. In the case of a container like EventInfo.js, the local state copy of what is in the redux store can be eliminated because it will be provided correctly by redux once the api call returns. Then componentWillReceiveProps can be eliminated without introducing issues with replacing it with componentDidUpdate.

Oh my gosh I feel like we just had a cosmic moment with the "put it in the reducer" intersection. Yes, no single source of truth for state isn't great. I like where you're thinking and may have overcomplicated or gone a bit too architecty in my response. Curious to also hear thoughts from others

I know there have been quite a few topics to give opinions on here @jmpainter @Sherspock @joaopapereira... sorry I haven't been able to write anything till now.

You bring up a lot of good points @Sherspock, and I appreciate that. There are definitely many things that can be improved/questioned in the code base. Several of them @joaopapereira has brought up.

Some of the logic that doesn't seem coherent @Sherspock was decided on the fly, by a group of developers, some of which are not actively taking part in the process at the moment, not uncommon.
We, as you know, rely on others coming in and questioning that logic, and opening tickets/putting in PRs to deal with it.

At the moment, I am mostly focused on trying to get things checked off blocking us from going to production. Is anything you mentioned ☝️ something that would block that, or could it be dealt with in parallel, or even after going to production?

There is also work to be done to ensure there are issues for people to work on, I have several developers getting in contact with me wanting to know what to do.

I feel like it would be nice to come up with an answer to this potential issue, but maybe it doesn't take the highest priority, but happy to discuss it if others disagree.

I would encourage people to come to the meetings, if at all possible, Tuesdays 4:45pm UTC, and also to feel free to take the liberty to open new issues. We also have the next client meeting with @tansaku on Wednesday 3:15pm UTC barring any last minute changes, where we can show the latest changes, and take the next steps forward.