Wrap all tests in strictmode

[tstirrat15]

Motivation

This should help with development against future versions of React and enforces best practices generally.

Changes

• Add test utils file with renderStrict and serverRenderStrict functions
• Implement those functions across all tests

Testing

Code review. See that tests pass.

[tstirrat15]

Though one thing I'm curious about is that it seems that the arity of renderStrict is wrong, and yet tests still pass. Any ideas there?

I'm also not seeing a single warning on output, which seems strange.

[tstirrat15]

Mmmk. I wasn't calling the callback, which was one problem, and another problem is that the peer dependency specified in package.json is an old version of react which doesn't have strictmode, so the utils file is doing exactly as expected and making strictmode a passthrough component. I'll verify that the behavior is correct with a recent version of react and then we can figure out how we want to run tests against those versions of react.

[tstirrat15]

It seems I'll have to rewrite some of the tests as well. They aren't playing nicely with React 16. I'll try and get around to this on Monday.

[edorivai]

Thanks a lot! Looking forward to it.

[tstirrat15]

firstChild was null, which meant that the statement was erroring out. Is this a good enough equivalent, or should it be node.firstChild && node.firstChild.innerHtml || ''?

[edorivai]

I'm a bit confused. Do you mean this test was broken before your changes? Or did this break by adding renderStrict?
In any case, I think node.innerHTML should be fine.

[tstirrat15]

Same thing here.

[tstirrat15]

@edorivai this should be ready for review now. I left a couple of comments on the substantive changes I made to the tests.

It seems like the tests should ideally be run against multiple versions of React, unless the plan is to move to React 16 as a part of v2.

[edorivai]

It seems like the tests should ideally be run against multiple versions of React, unless the plan is to move to React 16 as a part of v2.

That is actually a good point. Thing is, we plan to add a hooks API in 2.1, but this would be a separate bundle (like react-media/hooks). That hooks bundle would support >=16.7, but the main bundle could still support >=15 || ^0.14.7. Not sure what would be the correct version to require in the peerDependency in that case... 🤔

[edorivai]

I left one suggestion which you can apply, other than that everything looks good to me!

[edorivai]

I'm a bit confused. Do you mean this test was broken before your changes? Or did this break by adding renderStrict?
In any case, I think node.innerHTML should be fine.

[tstirrat15]

It wasn't broken before my changes. I think renderStrict did something, though it could also be the react 15->16 update if there were changes in how the react tree is committed to the DOM.

I'm more familiar with the enzyme method of writing tests, where it'd be pretty easy to say .find('div').text().to.equal('hello'), and the find statement will traverse and search the tree for you. Is there a way to do something similar in JSDOM? My concern is that this test wouldn't actually fail...

[edorivai]

I'm not that familiar with JSDOM myself, but it should be relatively straightforward to verify whether the test would fail, right? Let me know if you need help with verifying that!

[tstirrat15]

I verified it by removing firstChild from each of the tests and seeing that the suite still passes. I think this is good to go.

[edorivai]

Yep, looks good to me. Thanks a lot!