UIP-1164 (OR): Provide easy access to DOM node in ValidationUtil.warn messages

[kaanaksoy-wk]

This pull request is connected with UIP-1164 (WSD)

Ultimate problem:

ValidationUtil.warn should take an optional Element parameter which will be console.log/.warn'd to the browser console, allowing consumers to navigate to the offending DOM node in the inspector.

All usages of ValidationUtil.warn should be updated to pass in the corresponding DOM node, where applicable.

How it was fixed:

• Because ValidationUtil.warn is often used in components' render methods, it cannot use the findDomNode method, since it causes the following error message:

render() should be a pure function of props and state. It should never access something that requires stale data from the previous render, such as refs. Move this logic to componentDidMount and componentDidUpdate instead.

• To get around this issue, findDomNode will no longer be called within the ValidationUtil.warn method. As a result, the component will be logged to the console rather than the DOM node itself.

• Make the ValidationUtil.warn method accept an optional second parameter for a component.

• Following the standard warning message, log the component itself as well as its props in a collapsed group.

Testing suggestions:

• Verify that all current tests continue to pass.

Potential areas of regression:

• N/A

---

FYA: @greglittlefield-wf @aaronlademann-wf @jacehensley-wf @clairesarsam-wf @joelleibow-wf

[aviary2-wf]

Raven

Number of Findings: 0

[codecov-io]

Codecov Report

Merging #60 into master will decrease coverage by 0.07%.
The diff coverage is 90%.

@@            Coverage Diff             @@
##           master      #60      +/-   ##
==========================================
- Coverage   97.73%   97.67%   -0.06%     
==========================================
  Files          28       28              
  Lines        1365     1372       +7     
==========================================
+ Hits         1334     1340       +6     
- Misses         31       32       +1

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1986adf...8e3aca2. Read the comment docs.

[aaronlademann-wf]

Can we "pretty print" these props?

[greglittlefield-wf]

UiProps.toString has pretty-printing built in, so this will pretty-print them.

[greglittlefield-wf]

If we're expecting a component, we can type this to be react.Component.

[greglittlefield-wf]

Hm, or I wonder if we should accept ReactElements as well (would we ever want to print just the child?)...

Could do something like:

static bool warn(String message, [dynamic data]) {
  ...

  if (data != null) {
    window.console.groupCollapsed('(Warning info)');
    window.console.log(data);

    if (isValidElement(data)) {
      window.console.log('props: ${prettyPrintMap(getProps(data))}');
    } else if (data is react.Component) {
      window.console.log('props: ${prettyPrintMap(data.props)}');
    }
  }

 ...
}

[greglittlefield-wf]

Not trying to move the goalposts of this PR, just putting it out there as an idea 😃.

[kaanaksoy-wk]

@greglittlefield-wf Do I need to use prettyPrintMap in both cases? I thought that the toString method on UiProps already handled that for us.

[greglittlefield-wf]

Nope, you're right. The only case it'd be needed is if they're using plain react-dart component, but we don't really have to worry about that.

[greglittlefield-wf]

+1

[aaronlademann-wf]

QA +1

• Testing instruction
• Dev +1's
• Dev/QA +10
• Unit tests created/updated
• All unit tests pass
• Rosie ran/Rosie comment displays expected info
• Dependency Scan Clean

Merging.