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

Fix issue with stale values while mutating rows in MultiColumnField #3025

Merged
merged 2 commits into from Oct 24, 2019

Conversation

rohitkrai03
Copy link
Contributor

@rohitkrai03 rohitkrai03 commented Oct 21, 2019

Fixes bug where mutation on rows would result in stale values in MultiColumnFieldRow.

This PR -

  • Adds value prop in TextInput component to make it controlled even if field.value is undefined.
  • Fixes a bug in Dropdown component where the title would not update even afetr the selectedKey is changed in componentWillRecieveProps.
  • Removes the line to disable es-lint in MultiColumnField component.

@openshift-ci-robot openshift-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. component/dev-console Related to dev-console approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Oct 21, 2019
@debsmita1
Copy link
Contributor

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 21, 2019
@rohitkrai03
Copy link
Contributor Author

/lgtm cancel

@openshift-ci-robot openshift-ci-robot removed lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Oct 21, 2019
@@ -33,8 +33,7 @@ const MultiColumnField: React.FC<MultiColumnFieldProps> = ({
{fieldValue.length > 0 &&
fieldValue.map((value, index) => (
<MultiColumnFieldRow
// eslint-disable-next-line react/no-array-index-key
key={`multi-column-field-row-${index}`} // There is no other usable value for key prop in this case.
key={`multi-column-field-row-${JSON.stringify(value)}`}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there may be an issue with this - after I wrote the answer below, I started trying to think of what types value could be.... it occurs to me, it's probably an object for the entire row, no? If that's the case, we're likely looking at massive remounts. Every time some value is changed it remounts the whole row.

Original Thought:
I think this is mostly okay... This is a generic component, so technically, you could have multiple same-value items. If this conflict hits, it won't render the 2nd one and will log the error in the console; "children must have unique react keys" or something to that extent.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I ran into the remount issue while testing with InputField which affects performance drastically. That is why I had to cancel lgtm. Didn't notice the performance issue earlier as I was testing with dropdowns primarily.

Trying to find a better way to make the keys unique. But don't think we have much option in this case. Can't even use uuids as it would affect performance similar to what using values does.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to pass in some sort of keyId prop to the component? That way the mounting code can specific which part of the object is the key. If one is not provided, we could use index as a fallback? Probably just want to force it - we don't use this component a ton right now, so hopefully we could just update all of them at the same time.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need the keys to be unique for each row. Even if we send a keyId prop from parent component, it will be unique for the whole MultiColumnField component and not for each rows.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry - I think we are having a miscommunication, here is a fake snippet to explain what I mean:

{fieldValue.length > 0 &&
  fieldValue.map((value, index) => (
    <MultiColumnFieldRow
      key={`multi-column-field-row-${value[props.keyId]}`} // << It's a prop identifier
      name={name}
      rowIndex={index}
      onDelete={() => remove(index)}
      isReadOnly={isReadOnly}
    >
      {children}
    </MultiColumnFieldRow>
  ))}

EDIT: If the confusion was I called the prop values - that was a typo. It was meant to be value. I edited the snippet above.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, fieldValue is an array of objects and value would be something like { name: foo, type: bar }. You mean to say we add keyId in value object? The problem in that case would be we need to again generate those keyIds for each new row that user adds as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And if you meant using the value for a key like name in an object {name: foo, type: bar}, we'll have the same remount issue as the value of name will change on every keystroke of user.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, react keys are almost always an 'id' field or something similar. They are used to keep track of lists of items. It should ideally come from data.

What I am suggesting here is that we allow the MultiColumnField component to take a prop-name (my suggestion for the name of this prop was keyId) to allow for the caller of the component (whom theoretically would know their data) to make unique ids.

Now, the caller can select a prop from existing data, or they can create one by combining props together into a new object property in their data (to which they specify as their keyId).

Surely the data we are getting in View/Edit Permissions form is not just name and permission level. If it is, perhaps on retrieve of fetch we generate one (which can be an index number for all matching sakes) which does not change for the life of the page. Each time a new one is added, we generate a new one.

The key content does not matter, it just needs to be unique (and does not change during the operation of the item/field/component).

@openshift-ci-robot openshift-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Oct 21, 2019
@@ -1,5 +1,6 @@
import * as React from 'react';
import * as _ from 'lodash';
import uuid from 'uuid/v4';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

uuid/v4?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this even a dependency - I can't find it.

// Add a unique key to the object for keyed react list rendering
const keyedFieldValues = fieldValues.map((value) => {
if (!('uniqueKeyId' in value)) {
value.uniqueKeyId = uuid();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the public record - I still feel this is not the best way to do this - I just don't have a better idea off the top of my head. I feel there needs to be some external creation of an id - but @rohitkrai03 and I had a conversation about it and feel there is a lot of factors that go into this - and the internal functionality of this component being known externally is subject to coupling.

Since the external API of this component is to allow for multiple "on the fly" rows of the simple elements provided by the wrapping component, the ownership of "handling multiple rows" falls on this component and not the calling code.

@openshift-ci-robot openshift-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. component/core Related to console core functionality and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Oct 22, 2019
@rohitkrai03
Copy link
Contributor Author

/assign @christianvogt

@rohitkrai03 rohitkrai03 changed the title Remove use of index in MultiColumnFieldRow Fix issue with stale values while mutating rows in MultiColumnField Oct 22, 2019
@andrewballantyne
Copy link
Contributor

LGTM - I will wait upon other reviews given the change in direction.

@@ -34,8 +34,7 @@ const MultiColumnField: React.FC<MultiColumnFieldProps> = ({
{fieldValue.length > 0 &&
fieldValue.map((value, index) => (
<MultiColumnFieldRow
// eslint-disable-next-line react/no-array-index-key
key={`multi-column-field-row-${index}`} // There is no other usable value for key prop in this case.
key={`multi-column-field-row-${index.toString()}`} // There is no other usable value for key prop in this case.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
key={`multi-column-field-row-${index.toString()}`} // There is no other usable value for key prop in this case.
key={index.toString()} // There is no other usable value for key prop in this case.

if (selectedKey !== this.props.selectedKey) {
this.setState({ selectedKey });
this.setState({ selectedKey, title: items[selectedKey] });
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is right because props.title is also an option. Also if props.noSelection it seems that props.title is also used.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it will be an issue since logic to handle new title prop is there on line 268, under componentWillRecieveProps of Dropdown component.

And props.noSelection is only relevant when user clicks on the items in dropdown

@rohitkrai03
Copy link
Contributor Author

/test e2e-gcp

@rohitkrai03
Copy link
Contributor Author

/test e2e-aws-console-olm

@christianvogt
Copy link
Contributor

/lgtm
/approve

@christianvogt
Copy link
Contributor

/retest

@christianvogt
Copy link
Contributor

/lgtm

@openshift-ci-robot openshift-ci-robot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Oct 24, 2019
@christianvogt
Copy link
Contributor

/approve

@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: christianvogt, debsmita1

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-robot openshift-merge-robot merged commit afb10c2 into openshift:master Oct 24, 2019
@spadgett spadgett added this to the v4.3 milestone Oct 24, 2019
@rohitkrai03 rohitkrai03 deleted the mcf-bug-fix branch March 30, 2020 15:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. component/core Related to console core functionality component/dev-console Related to dev-console lgtm Indicates that a PR is ready to be merged. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants