SS-30102: Add react-lifecycles-compat polyfill to FDT to support v1.1…

[pradeepnschrodinger]

Description

The master branch of FDT doesn't work in LD due to a recent release.
Breakage happens because the change affected when FixedDataTableContainer dispatched the PROP_CHANGE action.

Basically the redux store is updated in response to prop changes only AFTER rendering is done, because the prop change redux action is dispatched in its componentDidUpdate.
This means, although the table gets rendered with new props, components within the tree won't get fully updated because the internal store hasn't updated yet.

Now during this time period, LD breaks!, and it has something to do with context.
The cells use data from the context, but it won't match the data in it's props, because FDT hasn't rendered them with the latest props yet (the internal store is still old).
Thus, somewhere in the cell renderer in LD, this leads to a script issue. I'll put up a CR in LD side to show/fix this.

I can't really think of a clean fix in the FDT side, unless we revert the usage of componentWillReceiveProps in FixedDataTableContainer.
For now, I've NOT done this in this CR as I wanted to explain and decide what approach we should go with.

---

I've also added the react-lifecycles-compat polyfill to FixedDataTableCell.
This was done because the cell component updates its state in componentDidUpdate. This isn't efficient since it leads to multiple renders and delayed state updates.
This might also lead to problems in LD for reasons explained above; but I haven't found out if this hunch is true because because the container problem above might have masked it.
Anyway, I used getDerivedStateFromProps to calculate the new state, ensuring the state is updated before the corresponding render, and then added the polyfill to the component to make sure it works in older versions of React as well.

How Has This Been Tested?

Tested in both LD and our examples, especially the reorder and resize column examples.

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

• My code follows the code style of this project.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have read the CONTRIBUTING document.

[pradeepnschrodinger]

Yet another bug, the order was changed in #509, and lead to other issues.

[pradeepnschrodinger]

There's no net change in the function, I just reordered statements to make it cleaner.

[wcjordan]

so is it just changing componentDidUpdate to getDerivedStateFromProps + reordering?

[pradeepnschrodinger]

Yep. This ensures a single render after state update in response to prop change.

[wcjordan]

cool. But to ensure I understand, we still get 2 rerenders, one for prop changes to the top level component, and another after the redux action is done?

[wcjordan]

Thanks @pradeepnschrodinger for the detailed explanation. So if this change breaks LD is there any chance it would break other usage?

It looks from my understanding like the break comes because we're now rendering twice... once from the initial props change, and then again from the redux store updating. Is that correct? And the breaks come when logic doesn't handle possible intermediate states?
Re-rendering twice seems not great. I'm surprised React didn't propose a way to handle this case when they switched lifecycle methods up.

[wcjordan]

no need to check for change anymore?

[pradeepnschrodinger]

Nope, because the this._didScroll already compares the scroll props before dispatching actions.

Furthermore, I believe comparing just the prop object's reference in order to skip actions might lead to bugs in the future (like if the user just changed the classname prop but the action still fires because prop reference will get changed). So it's better if the action itself just selects and compares the props it needs.

[wcjordan]

so is it just changing componentDidUpdate to getDerivedStateFromProps + reordering?

[pradeepnschrodinger]

So if this change breaks LD is there any chance it would break other usage?

Yep, if the library users have usages of context in their app.

It looks from my understanding like the break comes because we're now rendering twice... once from the initial props change, and then again from the redux store updating. Is that correct? And the breaks come when logic doesn't handle possible intermediate states?

Yes. To give a specific example, consider LD using FDT.
FDT considers columns as props. The store takes the props and uses it construct column information (and related fields) which'll be used to render the rows/columns.

When a LR is switched, the column props are changed leading to FDT being rendered.
Now, in this initial render, none of the store is updated. Since the store is also what holds the column information, FDT just renders the old rows/columns. The cell renderers will then be called down the tree, but still with the old data.
But the cell components might fail to render correctly because the data it specifically requires is no longer present in the current LR (due to the LR switch), and hence we say it's in an invalid intermediate state. The CR I posed in the LD side handles this specific state.

After all this, there's a second render which is triggered in response to store update through prop changes. In this render the column information matches the data in LD and should be a successful run. Sadly, FDT is unable to recover because the render tree is broken at this point, due to the previous render.

Re-rendering twice seems not great. I'm surprised React didn't propose a way to handle this case when they switched lifecycle methods up.

Agreed. I think a better solution would be to just use UNSAFE_componentWillReceiveProps for updating the store. It should work even on React v17, and give us time to think of a better solution while making sure FDT works for all library users (including LD).
This essentially reverts back our original behavior of only rendering once per prop change.
Additionally, since it's marked UNSAFE, React won't show console warnings.

[pradeepnschrodinger]

So if this change breaks LD is there any chance it would break other usage?

Yep, if the library users have usages of context in their app.

I take this back. It doesn't break only through context.
Since the cell renderers have stale props that won't match the current app state, I feel this CAN lead to other errors in general.

[wcjordan]

Sounds good. Do you want to switch to using UNSAFE_componentWillReceiveProp? After that could you file an SS ticket for removing the use of that method w/ a better replacement and send it to me?

[wcjordan]

Assuming we've addressed all concerns, lgtm

[pradeepnschrodinger]

Whoops, there's one more issue.
The plan to UNSAFE_componentWillReceiveProps won't work with react versions less than v16.3 because the new aliases are introduced only from v16.3 onwards.
So until we bump up FDT's dependency of react to >= 16.3, we can't guarantee this works for all users.

Instead, we can just use 'componentWillReceiveProps` for now. Is that okay?

[wcjordan]

Ah lame. We could do that, but will still get the deprecation warning. I think that's fine though...

I wonder if we could add something like.

If !ReactComponent.prototype.UNSAFE_componentWillReceiveProps
   ReactComponent.prototype.UNSAFE_componentWillReceiveProps = ReactComponent.prototype.componentWillReceiveProps

and then use UNSAFE_componentWillReceiveProps, but still have it work on older React versions

[pradeepnschrodinger]

I wonder if we could add something like.

If !ReactComponent.prototype.UNSAFE_componentWillReceiveProps
   ReactComponent.prototype.UNSAFE_componentWillReceiveProps = ReactComponent.prototype.componentWillReceiveProps

and then use UNSAFE_componentWillReceiveProps, but still have it work on older React versions

I couldn't figure out a neat way to detect the life cycle methods.
Also react seems to call them both if they're both defined...

[wcjordan]

Oh lame. Alright, let's go with componentWillReceiveProps & the deprecation notice for now.

Could you comment on the PR / ticket which updated the lifecycle methods to ensure anyone watching that is looped in? Let's also carve out a task for a better long-term fix.

[pradeepnschrodinger]

Cool, I've ticked out SS-30193 and linked to this PR.