onefinestay / react-daterange-picker

Other
563 stars 208 forks source link

Update to React 0.14 #79

Closed rolyatmax closed 8 years ago

rolyatmax commented 8 years ago

Splitting this out from https://github.com/onefinestay/react-daterange-picker/pull/78.

rolyatmax commented 8 years ago

@AlanFoster Take a look and let me know what you think!

AlanFoster commented 8 years ago

Still waiting on Travis integration to be enabled unfortunately, so running tests is a manual process for now. #83 is keeping track of this however.

After a rebase on master, are all tests green?

rolyatmax commented 8 years ago

Ah, I see - are there instructions for how to run the tests?

AlanFoster commented 8 years ago

@rolyatmax I've just updated the readme file now under #86. Ping me if you need any help

rolyatmax commented 8 years ago

@AlanFoster et al

Running tests - a couple failing tests which all seem to be related to changes in findAllInRenderedTree: https://facebook.github.io/react/blog/2015/10/07/react-v0.14.html#breaking-changes

My best guess is that they have something to do with this line: https://github.com/onefinestay/react-daterange-picker/blob/master/src/calendar/tests/CalendarDate.spec.js#L64

All tests using the DocumentRenderer are failing.

I don't have a ton of experience with using React's Test Utils though. Any thoughts?

Pushing up my latest, and here's the test output:

SUMMARY:
✔ 192 tests completed
✖ 7 tests failed

FAILED TESTS:
  The CalendarDate Component
    handles touch events
      ✖ by calling props.onHighlightDate after an interaction
        PhantomJS 1.9.8 (Mac OS X 0.0.0)
      Error: Invariant Violation: findAllInRenderedTree(...): instance must be a composite component
          at /Users/taylorbaldwin/sites/react-daterange-picker/src/tests.webpack.js:1573
          at /Users/taylorbaldwin/sites/react-daterange-picker/src/tests.webpack.js:20231
          at /Users/taylorbaldwin/sites/react-daterange-picker/src/tests.webpack.js:20277
          at /Users/taylorbaldwin/sites/react-daterange-picker/src/tests.webpack.js:20287
          at /Users/taylorbaldwin/sites/react-daterange-picker/src/tests.webpack.js:179
          at /Users/taylorbaldwin/sites/react-daterange-picker/src/tests.webpack.js:400
      Expected spy unknown to have been called.
          at /Users/taylorbaldwin/sites/react-daterange-picker/src/tests.webpack.js:408

      ✖ by calling props.onSelectDate after an interaction
        PhantomJS 1.9.8 (Mac OS X 0.0.0)
      Error: Invariant Violation: findAllInRenderedTree(...): instance must be a composite component
          at /Users/taylorbaldwin/sites/react-daterange-picker/src/tests.webpack.js:1573
          at /Users/taylorbaldwin/sites/react-daterange-picker/src/tests.webpack.js:20231
          at /Users/taylorbaldwin/sites/react-daterange-picker/src/tests.webpack.js:20277
          at /Users/taylorbaldwin/sites/react-daterange-picker/src/tests.webpack.js:20287
          at /Users/taylorbaldwin/sites/react-daterange-picker/src/tests.webpack.js:179
          at /Users/taylorbaldwin/sites/react-daterange-picker/src/tests.webpack.js:400
      Expected spy unknown to have been called.
          at /Users/taylorbaldwin/sites/react-daterange-picker/src/tests.webpack.js:412

    handles mouse events
      ✖ by calling props.onHighlightDate after a mouse enter
        PhantomJS 1.9.8 (Mac OS X 0.0.0)
      Error: Invariant Violation: findAllInRenderedTree(...): instance must be a composite component
          at /Users/taylorbaldwin/sites/react-daterange-picker/src/tests.webpack.js:1573
          at /Users/taylorbaldwin/sites/react-daterange-picker/src/tests.webpack.js:20231
          at /Users/taylorbaldwin/sites/react-daterange-picker/src/tests.webpack.js:20277
          at /Users/taylorbaldwin/sites/react-daterange-picker/src/tests.webpack.js:20287
          at /Users/taylorbaldwin/sites/react-daterange-picker/src/tests.webpack.js:179
          at /Users/taylorbaldwin/sites/react-daterange-picker/src/tests.webpack.js:424
      TypeError: 'undefined' is not an object (evaluating 'domComponentOrNode.tagName')
          at /Users/taylorbaldwin/sites/react-daterange-picker/src/tests.webpack.js:20536
          at /Users/taylorbaldwin/sites/react-daterange-picker/src/tests.webpack.js:429

      ✖ by calling props.onSelectDate after mouse down + mouse leave
        PhantomJS 1.9.8 (Mac OS X 0.0.0)
      Error: Invariant Violation: findAllInRenderedTree(...): instance must be a composite component
          at /Users/taylorbaldwin/sites/react-daterange-picker/src/tests.webpack.js:1573
          at /Users/taylorbaldwin/sites/react-daterange-picker/src/tests.webpack.js:20231
          at /Users/taylorbaldwin/sites/react-daterange-picker/src/tests.webpack.js:20277
          at /Users/taylorbaldwin/sites/react-daterange-picker/src/tests.webpack.js:20287
          at /Users/taylorbaldwin/sites/react-daterange-picker/src/tests.webpack.js:179
          at /Users/taylorbaldwin/sites/react-daterange-picker/src/tests.webpack.js:424
      TypeError: 'undefined' is not an object (evaluating 'domComponentOrNode.tagName')
          at /Users/taylorbaldwin/sites/react-daterange-picker/src/tests.webpack.js:20461
          at /Users/taylorbaldwin/sites/react-daterange-picker/src/tests.webpack.js:434

      ✖ by calling props.onUnHighlightDate after a mouse leave
        PhantomJS 1.9.8 (Mac OS X 0.0.0)
      Error: Invariant Violation: findAllInRenderedTree(...): instance must be a composite component
          at /Users/taylorbaldwin/sites/react-daterange-picker/src/tests.webpack.js:1573
          at /Users/taylorbaldwin/sites/react-daterange-picker/src/tests.webpack.js:20231
          at /Users/taylorbaldwin/sites/react-daterange-picker/src/tests.webpack.js:20277
          at /Users/taylorbaldwin/sites/react-daterange-picker/src/tests.webpack.js:20287
          at /Users/taylorbaldwin/sites/react-daterange-picker/src/tests.webpack.js:179
          at /Users/taylorbaldwin/sites/react-daterange-picker/src/tests.webpack.js:424
      TypeError: 'undefined' is not an object (evaluating 'domComponentOrNode.tagName')
          at /Users/taylorbaldwin/sites/react-daterange-picker/src/tests.webpack.js:20536
          at /Users/taylorbaldwin/sites/react-daterange-picker/src/tests.webpack.js:440

      ✖ by calling props.onSelectDate after mouse down + mouse up
        PhantomJS 1.9.8 (Mac OS X 0.0.0)
      Error: Invariant Violation: findAllInRenderedTree(...): instance must be a composite component
          at /Users/taylorbaldwin/sites/react-daterange-picker/src/tests.webpack.js:1573
          at /Users/taylorbaldwin/sites/react-daterange-picker/src/tests.webpack.js:20231
          at /Users/taylorbaldwin/sites/react-daterange-picker/src/tests.webpack.js:20277
          at /Users/taylorbaldwin/sites/react-daterange-picker/src/tests.webpack.js:20287
          at /Users/taylorbaldwin/sites/react-daterange-picker/src/tests.webpack.js:179
          at /Users/taylorbaldwin/sites/react-daterange-picker/src/tests.webpack.js:424
      TypeError: 'undefined' is not an object (evaluating 'domComponentOrNode.tagName')
          at /Users/taylorbaldwin/sites/react-daterange-picker/src/tests.webpack.js:20461
          at /Users/taylorbaldwin/sites/react-daterange-picker/src/tests.webpack.js:445

  The Legend component
    ✖ creates extra lis based on the props.stateDefinitions
      PhantomJS 1.9.8 (Mac OS X 0.0.0)
    Error: Invariant Violation: findAllInRenderedTree(...): instance must be a composite component
        at /Users/taylorbaldwin/sites/react-daterange-picker/src/tests.webpack.js:1573
        at /Users/taylorbaldwin/sites/react-daterange-picker/src/tests.webpack.js:20231
        at /Users/taylorbaldwin/sites/react-daterange-picker/src/tests.webpack.js:20277
        at /Users/taylorbaldwin/sites/react-daterange-picker/src/tests.webpack.js:41504
rolyatmax commented 8 years ago

Also, haven't been able to get tests passing on current master :confused: hahaha - but that might be because there's something wrong with my setup.

AlanFoster commented 8 years ago

@rolyatmax They unfortunately fail currently because the package.json file is too leniant, it should be updated to only allow React 0.12.x or 0.13.x; ie running npm install react@0.13.3 and npm run test should yield passing tests.

To get the tests running with 0.14.x however; Would you be okay updating

  this.renderedComponent = TestUtils.findRenderedDOMComponentWithTag(renderedTable, 'td');

to be

this.renderedComponent = ReactDOM.findDOMNode(TestUtils.findRenderedComponentWithType(renderedTable, CalendarDate));
rolyatmax commented 8 years ago

Can do! I'll submit updates shortly! :-)

On Oct 23, 2015, at 12:07 PM, Alan Foster notifications@github.com wrote:

@rolyatmax They unfortunately fail currently because the package.json file is too leniant, it should be updated to only allow React 0.12.x or 0.13.x; ie running npm install react@0.13.3 and npm run test should yield passing tests.

To get the tests running with 0.14.x however; Would you be okay updating

this.renderedComponent = TestUtils.findRenderedDOMComponentWithTag(renderedTable, 'td'); to be

this.renderedComponent = ReactDOM.findDOMNode(TestUtils.findRenderedComponentWithType(renderedTable, CalendarDate)); — Reply to this email directly or view it on GitHub.

AlanFoster commented 8 years ago

@rolyatmax Awesome, thanks :)

rolyatmax commented 8 years ago

@AlanFoster Hmmm - that doesn't seem to be working for me. Have you pulled this down and tried those changes? Sorry I'm not more help - I'm not terribly familiar with the TestUtils.

thathenderson commented 8 years ago

@rolyatmax @AlanFoster It looks like TestUtils.renderIntoDocument now returns a DOM node (react#4692), and findRenderedComponentWithType and findRenderedDOMComponentWithTag both expect a ReactComponent to traverse.

Playing around with it on my own, I had success simply using getElementsByTagName on the rendered <table> instead of relying on TestUtils:

this.renderedComponent = renderedTable.getElementsByTagName("td")[0];

I also noticed the 'creates extra lis based on the props.stateDefinitions' test in the Legend spec was failing. Doing the following seemed to fix it:

var lis = ReactDOM.findDOMNode(this.renderedComponent);
expect(lis.childNodes.length).toBe(3);

var spans = lis.childNodes[1].getElementsByTagName('span');
expect(spans.length).toBe(2);

expect(spans[0].style.backgroundColor).toBe('blue');
expect(spans[1].textContent).toBe('abc');
rolyatmax commented 8 years ago

:heart: :heart: @thathenderson :heart: :heart:

This is great. Thanks!!

Squashed some updates into that last commit.

Tests and linting both pass.

SUMMARY:
✔ 199 tests completed

@AlanFoster Let me know what you think

stevewillard commented 8 years ago

Any chance we can get this merged in?

damountie commented 8 years ago

Testing this has been complicated for me by unrelated React 0.14-upgrade stuff elsewhere in my project's code-base; but as much as I am able to test, this PR seems good for me and worth merging. :+1:

thathenderson commented 8 years ago

Sorry, as @AlanFoster pointed out, I missed something. renderIntoDocument returns a DOM node only when the rendered component is wrapped in a DOM node. That's why I originally included findDOMNode.

You can refactor it to remove findDOMNode and the ReactDOM dependency if you wrap <Legend /> in a div:

this.useDocumentRenderer = (props) => {
  const domComponent = TestUtils.renderIntoDocument(<div>{getLegend(props)}</div>);
  this.renderedComponent = domComponent.childNodes[0];
};
rolyatmax commented 8 years ago

@AlanFoster @thathenderson Take a look at that updated last commit and let me know how that looks.

All tests and linting are passing.

thathenderson commented 8 years ago

Looks great to me!

AlanFoster commented 8 years ago

@rolyatmax Could you rebase against master please, there's a small merge conflict to resolve

rolyatmax commented 8 years ago

@AlanFoster rebased

AlanFoster commented 8 years ago

@rolyatmax Perfect, thanks. All green on travis now too :+1: I'll try to get all of this work merged and deployed today / tomorrow. Thanks for the great work!

rolyatmax commented 8 years ago

@AlanFoster :+1: :+1: :+1: :+1:

rolyatmax commented 8 years ago

Rebased again to resolve conflicts. All tests and linting pass.

stevewillard commented 8 years ago

Any chance for a merge? Looking to upgrade our version of React, but this is holding us back

zoopoetics commented 8 years ago

+1 for merge :) We're also upgrading to React 0.14, and this is the only dependency holding us back.

AlanFoster commented 8 years ago

For visibility I plan to merge #99 before releasing 0.12.2, then I shall be merging this PR with the intent of a 1.0.0 release.

Would appreciate a rebase for this PR :+1:

rolyatmax commented 8 years ago

@AlanFoster rebased!

stevewillard commented 8 years ago

@rolyatmax can you rebase again? Looks like 0.12.2/1.0.0 are close to be ready

rolyatmax commented 8 years ago

Rebased - and added a commit to remove an unused var in the gulpfile to pass linting.

Tests/linting both pass.

AlanFoster commented 8 years ago

Thanks for all the work on this PR guys, v0.12.2 is now released - so let's get this merged in now! :+1:

nnarhinen commented 8 years ago

Can you push to NPM too?