AF-232 Expose render and unmountComponentAtNode #154
Conversation
Security InsightsNo security relevant content was detected by automated scans. Action Items
Questions or Comments? Reach out on HipChat: InfoSec Forum. |
2c6af28 to
ddc6ac3
Compare
ddc6ac3 to
11f23bb
Compare
Codecov Report
@@ Coverage Diff @@
## master #154 +/- ##
==========================================
+ Coverage 94.51% 94.52% +0.01%
==========================================
Files 33 34 +1
Lines 1601 1603 +2
==========================================
+ Hits 1513 1515 +2
Misses 88 88 |
lib/react_dom.dart
Outdated
| /// Renders the provided [vDomComponent] into the DOM in the provided [mountNode] | ||
| /// and returns a reference to it based on its type: | ||
| /// | ||
| /// 1. Returns an [Element] if [vDomComponent] is a [Dom] component _(e.g. [Dom.div])_. |
There was a problem hiding this comment.
#nit I think we can just say DOM component, since it doesn't necessarily have to come from [Dom].
lib/react_dom.dart
Outdated
| /// > Use [unmountComponentAtNode] to unmount the instance. | ||
| /// | ||
| /// > Proxies [react_dom.render]. | ||
| dynamic render(ReactElement vDomComponent, Element mountNode) { |
There was a problem hiding this comment.
I think it vDomComponent should be renamed to element to be consistent with our other naming conventions (or nextElement, which is what they call the variable in the React JS code).
lib/react_dom.dart
Outdated
| /// | ||
| /// > Proxies [react_dom.render]. | ||
| dynamic render(ReactElement vDomComponent, Element mountNode) { | ||
| if (vDomComponent == null) return null; |
There was a problem hiding this comment.
We don't need this check, since React handles this case. Plus, this would prevent the behavior of effectively unmounting a component by rendering null. Could you add a test for that case?
There was a problem hiding this comment.
@greglittlefield-wf it appears that the doc comment is not accurate.
If passed null, ReactDom.render throws an InvariantViolation:
'Invariant Violation: ReactDOM.render(): Invalid component element.'
I've added a test to verify that... but as far as I can tell - we don't have a way to create stateless components (or at least not one in which the return value would be ReactElement), so I've removed that bit from the doc comment.
There was a problem hiding this comment.
We don't have a way to create those, but that could change in the future, and you could also potentially get one from a JS library, so I think we should have retained that information. Not a big deal, though.
+ This would cause the component to be unmounted, so we should just let the proxied render method do its thing when either of the args are null.
|
@greglittlefield-wf all feedback addressed. |
|
QA +1
@Workiva/release-management-pp |
Ultimate problem:
We currently expose most of the commonly consumed methods from the
reactpackage, but not anything from itsreact_dom.dart. This forces consumers to importpackage:react/react_dom.dart, and therefore declarereactas a dependency for their application.However, this library is designed to act as an improved consumption experience for
reactconsumers, which should include insulation from internal breaking changes without having to change the upper bound of a dependency.How it was fixed:
react_dom.renderandreact_dom.unmountComponentAtNodemethods frompackage:react/react_dom.dart, with improved function / return signatures.For consumers, this should be the extent of the changes necessary to stop depending directly on
react:Testing suggestions:
Potential areas of regression:
None expected