Fix bug introduced in #677 #680

nfcampos · 2016-11-13T10:11:12Z

PR #677 introduced a bug where calling hasClass on a component that renders null would throw an exception. This fixes that.

nfcampos · 2016-11-13T10:11:45Z

src/MountedTraversal.js

@@ -47,6 +47,9 @@ export function instEqual(a, b, lenComp) {

 export function instHasClassName(inst, className) {
  const node = findDOMNode(inst);
+  if (!node) { // inst renders null


Is it better to check !node or node === null here?

If the instance renders "nothing", it could return an empty array, or null, or undefined… is there perhaps a react method we could use, like React.Children.count or something, that would cover us here?

But can a react component return anything other than null or a react element? Besides, what we're talking about here is the output of findDOMNode, can it be anything other than a dom element or null, I dont know...

That's a good point - https://facebook.github.io/react/docs/react-dom.html#finddomnode makes it seem like it's only null or a node. I wonder if then it would be better to use === null specifically so that it blows up if React changes the behavior?

I agree, I've changed it to === null

nfcampos · 2016-11-15T21:18:19Z

same thing as in #679 (comment) applies here I believe, after #677 hasClass can be used with composites as long as they're class based (because its implementation relies on findDOMNode).
Should we introduce the logic similar to #679 or should we try to re-use it (eg. by calling getDOMNode in hasClass)?

ljharb · 2016-11-16T00:29:52Z

I think the two PRs can coexist - hasClass is a boolean method and it seems reasonable for it to return false here, and for getDOMNode to throw.

nfcampos · 2016-11-26T23:38:32Z

@ljharb You wouldn't expect this test to pass?

const Foo = () => <div className="bar" />
const wrapper = mount(<Foo />)
expect(wrapper.hasClass('bar')).to.equal(true)

ljharb · 2016-11-27T04:43:53Z

hmm, that's a good point. no, i wouldn't; that should work.

nfcampos · 2016-11-28T23:21:19Z

So what should we do here, throw if used on a SFC?

ljharb · 2016-11-28T23:57:17Z

Sorry, my comment was confusing. I would indeed expect that test to pass - so SFCs should work, and not throw.

If they won't work properly, then they should throw, of course.

nfcampos · 2016-12-12T21:53:25Z

@ljharb sorry for the late reply, I don't think there's any way to get a ref to the dom node rendered for a stateless component so I'll update the PR to make the method throw when used on a stateless component.

nfcampos · 2016-12-12T22:48:45Z

So, apparently the react documentation is not up-to-date or I misread it because findDOMNode does work on functional components, as such, I've updated the PR with tests for hasClass on SFCs and removed the previously added check on getDOMNode for SFCs. Thoughts?

ljharb

This seems good, but lets get lots of eyes on it prior to merging

ljharb · 2016-12-13T01:51:21Z

test/ReactWrapper-spec.jsx


-      it('should throw when wrapping an SFC', () => {
+      it('should return the outer most DOMComponent of the inner div wrapper', () => {


"outermost" is one word :-p

indeed it is ;) updated

aweary · 2016-12-13T16:04:03Z

test/ReactWrapper-spec.jsx

+
+      it('should throw when wrapping multiple elements', () => {
+        const wrapper = mount(<SFC />).find('span');
+        expect(() => wrapper.getDOMNode()).to.throw(Error);


Can we test the error message like we had before? Testing that it throws any error seems brittle.

Probably both would be good; but if we want our error tests to be more robust we should be making error subclasses, not doing string comparisons.

Error subclasses are a better solution, I'm on board with that approach. But a string comparison would be better than nothing, so I'm fine with doing that for now too.

nfcampos · 2016-12-13T21:15:06Z

FYI @ljharb @aweary https://twitter.com/dan_abramov/status/808781309745201152
given this, should we support these methods for SFCs while they work, potentially seeing unexpectedly them break in a way we can't fix with a future React release?

ljharb · 2016-12-13T21:17:40Z

Yes, if it works, we should support it for SFCs.

If it breaks in a major version (since it'd be a breaking change), we'll have to conditionally support it based on the react version.

nfcampos · 2016-12-13T21:24:10Z

@ljharb since we're relying on "accidental" non-public behaviour of the ReactDOM API it could theoretically break in a patch version

ljharb · 2016-12-13T22:01:16Z

Sadly "if it's reachable, it's public" isn't a philosophy React tends to follow, but in this case I think we can, since it would probably cause lots of breakage across the React ecosystem. I think we should be optimistic here that React will follow semver.

nfcampos · 2016-12-13T22:07:27Z

@ljharb I think we might be pretty alone in relying on this particular behaviour of findDOMNode, (especially because the usefulness of using this very convoluted way (it requires rendering the SFC as a child of a class component, ReactWrapperComponent for us, and accessing it from there) of accessing dom nodes for SFCs is next to null outside of testing (but very useful in testing obviously))

- passing test for composite component that renders composite component which itself renders a dom element - failing test for composite component that renders null

see https://facebook.github.io/react/docs/react-dom.html#finddomnode for return values of findDOMNode

…OMNode on SFCs

nfcampos · 2016-12-22T15:05:06Z

@ljharb @aweary I've added the error message testing, this should now be ready to merge

aweary

LGTM

devrelm · 2016-12-27T21:57:48Z

@ljharb Can we get a patch release to npm with these changes? We're going to have to pin to 2.6.0 as a few of our tests are currently broken due hasClass not working on empty nodes.

ljharb · 2016-12-27T22:56:56Z

Sure, I should be able to get one out today or tomorrow.

nfcampos commented Nov 13, 2016

View reviewed changes

ljharb added API: mount semver: patch labels Nov 13, 2016

nfcampos force-pushed the fix-677 branch from 5964f06 to c813c25 Compare November 13, 2016 18:08

ljharb approved these changes Nov 15, 2016

View reviewed changes

nfcampos force-pushed the fix-677 branch from e4f72a2 to 735b7ce Compare December 12, 2016 21:51

nfcampos requested a review from ljharb December 12, 2016 22:47

ljharb approved these changes Dec 13, 2016

View reviewed changes

aweary reviewed Dec 13, 2016

View reviewed changes

nfcampos force-pushed the fix-677 branch from d9c322c to 36fce11 Compare December 13, 2016 20:10

nfcampos added 5 commits December 22, 2016 14:02

added 2 more tests for hasClass on composite components

bfc6dd3

- passing test for composite component that renders composite component which itself renders a dom element - failing test for composite component that renders null

fix for calling hasClass on component that renders null

79a70c4

stricter condition following React documentationa

6456995

see https://facebook.github.io/react/docs/react-dom.html#finddomnode for return values of findDOMNode

Add test for hasClass used on SFCs, Remove limitation for use of getD…

4d0eb62

…OMNode on SFCs

Fix typo

cb1daad

nfcampos force-pushed the fix-677 branch from 36fce11 to cb1daad Compare December 22, 2016 14:11

Test error messages for getDOMNode

fc2fef0

nfcampos force-pushed the fix-677 branch from f7d302c to fc2fef0 Compare December 22, 2016 14:20

aweary approved these changes Dec 22, 2016

View reviewed changes

nfcampos merged commit 3d7b68b into master Dec 22, 2016

nfcampos deleted the fix-677 branch December 22, 2016 21:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix bug introduced in #677 #680

Fix bug introduced in #677 #680

nfcampos commented Nov 13, 2016 •

edited by blainekasten

Loading

nfcampos Nov 13, 2016

ljharb Nov 13, 2016

nfcampos Nov 13, 2016

ljharb Nov 13, 2016

nfcampos Nov 14, 2016

nfcampos commented Nov 15, 2016

ljharb commented Nov 16, 2016

nfcampos commented Nov 26, 2016

ljharb commented Nov 27, 2016

nfcampos commented Nov 28, 2016

ljharb commented Nov 28, 2016

nfcampos commented Dec 12, 2016

nfcampos commented Dec 12, 2016 •

edited

Loading

ljharb left a comment

ljharb Dec 13, 2016

nfcampos Dec 13, 2016

aweary Dec 13, 2016

ljharb Dec 13, 2016

aweary Dec 13, 2016

nfcampos commented Dec 13, 2016

ljharb commented Dec 13, 2016

nfcampos commented Dec 13, 2016

ljharb commented Dec 13, 2016

nfcampos commented Dec 13, 2016

nfcampos commented Dec 22, 2016

aweary left a comment

devrelm commented Dec 27, 2016

ljharb commented Dec 27, 2016


		it('should throw when wrapping an SFC', () => {
		it('should return the outer most DOMComponent of the inner div wrapper', () => {

Fix bug introduced in #677 #680

Fix bug introduced in #677 #680

Conversation

nfcampos commented Nov 13, 2016 • edited by blainekasten Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nfcampos commented Nov 15, 2016

ljharb commented Nov 16, 2016

nfcampos commented Nov 26, 2016

ljharb commented Nov 27, 2016

nfcampos commented Nov 28, 2016

ljharb commented Nov 28, 2016

nfcampos commented Dec 12, 2016

nfcampos commented Dec 12, 2016 • edited Loading

ljharb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nfcampos commented Dec 13, 2016

ljharb commented Dec 13, 2016

nfcampos commented Dec 13, 2016

ljharb commented Dec 13, 2016

nfcampos commented Dec 13, 2016

nfcampos commented Dec 22, 2016

aweary left a comment

Choose a reason for hiding this comment

devrelm commented Dec 27, 2016

ljharb commented Dec 27, 2016

nfcampos commented Nov 13, 2016 •

edited by blainekasten

Loading

nfcampos commented Dec 12, 2016 •

edited

Loading