node field/prop collisions

[rebeccamadsen]

Since we use the spread operator to include all the fields in a node or EnhancedNode after our props (in most or all cases?), it is possible we could have unintended collisions and clobbering of network canvas functionality. Some overriding of props is likely intentional (e.g. label, color, selected). Some is decidely not (e.g. meta, onSelected, allowDrag). Which is which is up for discussion.

This is related to our uid discussion. See the conversation here, with one idea being to prepend special props with an underscore.

[wwqrd]

This configurations allows you to override default properties. In cases where we don't want to do that we can change the position of the spread operator:

<EnhancedNode
  {...props}
  label="Non overridable field"
/>

Or in cases where we want to exclude specific properties from the component:

const foo = ({ dontIncludeThis, orThis, ...rest }) => (
  <EnhancedNode
    {...rest}
  />
);

[jthrilly]

I think reordering will work. @rebeccamadsen ?

[rebeccamadsen]

I think reordering is part of the solution. It would ensure the user can't override our system created props and cause a crash or otherwise adversely affect functionality of the app; it protects us. But without the documentation to warn a user which fields we are using, wouldn't we still run the risk of clobbering their data? It's a two way clobbering. Maybe they have their own use for a 'meta' field just as much as we do?

[jthrilly]

I think I am still in favor of our "special" properties having some kind of pattern to them. Either nc_prop or just _prop.

@wwqrd - this seems like the sort of suggestion you would hate. Can you think of a better way?

[bryfox]

I'm trying to understand the reason for spreading external data props into view components, rather than keeping things contained in something like a sourceData prop. With the node example:

• What view properties are we intending users to customize directly via externalData? Labels, for example, have a different mechanism (displayLabel).
• Should 'special' props (including uid) be exported with data? Currently, the export merges view information (like stageId) into the Node data (which I assume is intentional); put another way, is the merging of app & user data required?
• Can a user export data from the app, and source that as external data for another interview? (If so, does that make special naming (nc_*) an incomplete solution for this problem?)

I'm sure there are good answers to these, and we can't just move external data into a containing prop to solve the issue. Hopefully we can document the answers somewhere permanent (possibly wiki); a better understanding of the expected data flows would if nothing else help me during development & testing.

[wwqrd]

Is there any reason we can't just be explicit with props at the point of use? We know that Node only takes x, y, z parameters, so we don't need to pass it anything and everything from the source data in the first place.:

render() {
  return nodes.map(
    ({ name }) => (<EnhancedNode onClick={() => this.clickHandler()} name={name} />),
  );
}

This way it's going to be clear if props are going to clash, at which point maybe it's a decision about better names for HOC props.

---

The pattern arises out of the use of HOCs:

const Node = ({ name }) => (<div>{name}</div>);

const ComponentWithOnClick = (WrappedComponent) =>
  ({ onClick, ...rest }) => (<WrappedComponent onClick={onClick} {...rest} />);

const EnhancedNode = ComponentWithOnClick(Node);

...

render() {
  return nodes.map(
    node => (<EnhancedNode onClick={() => this.clickHandler()} {...node} />),
  );
}

if we group all those props with a name we get:

const Node = ({ node }) => (<div>{node.name}</div>);

const EnhancedNode = ComponentWithOnClick(Node);

...

render() {
  return nodes.map(
    node => (<EnhancedNode onClick={() => this.clickHandler()} node={node} />),
  );
}

or:

const Node = ({ name }) => (<div>{name}</div>);

const NodeWithOnClick = (Node) =>
  ({ onClick, node }) => (<Node onClick={onClick} {...node />);

const PanelWithOnClick = (Panel) =>
  ({ onClick, panel }) => (<Panel onClick={onClick} {...panel} />);

...

const EnhancedNode = NodeWithOnClick(Node);

...

render() {
  return nodes.map(
    node => (<EnhancedNode onClick={() => this.clickHandler()} node={node} />),
  );
}

[jthrilly]

Thanks for clarifying this - I don't know what I was thinking with my initial comments.

@wwqrd is absolutely correct that we should be only dealing with the props that the Node component actually needs. We know exactly what those props are - and there is no reason to think there should be collisions with user defined properties if we are explicit.