Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update to React 0.14 #79

Merged
merged 4 commits into from
Nov 7, 2015
Merged

Conversation

rolyatmax
Copy link
Contributor

Splitting this out from #78.

@rolyatmax
Copy link
Contributor Author

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

This was referenced Oct 22, 2015
@AlanFoster
Copy link
Member

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
Copy link
Contributor Author

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

@AlanFoster
Copy link
Member

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

@rolyatmax
Copy link
Contributor Author

@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
Copy link
Contributor Author

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

@AlanFoster
Copy link
Member

@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
Copy link
Contributor Author

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
Copy link
Member

@rolyatmax Awesome, thanks :)

@rolyatmax
Copy link
Contributor Author

@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
Copy link

@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
Copy link
Contributor Author

❤️ ❤️ @thathenderson ❤️ ❤️

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
Copy link

Any chance we can get this merged in?

@damountie
Copy link

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. 👍

expect(lis.length).toBe(3);
var spans = TestUtils.scryRenderedDOMComponentsWithTag(lis[1], 'span');

var lis = ReactDOM.findDOMNode(this.renderedComponent);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❓ Won't this.renderedComponent already be a DOM Node at this point; can we delete the findDOMNode call?

@thathenderson
Copy link

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
Copy link
Contributor Author

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

All tests and linting are passing.

@thathenderson
Copy link

Looks great to me!

@AlanFoster
Copy link
Member

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

@AlanFoster AlanFoster mentioned this pull request Oct 27, 2015
@rolyatmax rolyatmax force-pushed the tb-react014 branch 3 times, most recently from 5267a4c to bbde573 Compare October 27, 2015 13:53
@rolyatmax
Copy link
Contributor Author

@AlanFoster rebased

@AlanFoster
Copy link
Member

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

@rolyatmax
Copy link
Contributor Author

@AlanFoster 👍 👍 👍 👍

@rolyatmax
Copy link
Contributor Author

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

@stevewillard
Copy link

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

@zoopoetics
Copy link

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

@AlanFoster
Copy link
Member

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 👍

@rolyatmax
Copy link
Contributor Author

@AlanFoster rebased!

@stevewillard
Copy link

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

@rolyatmax
Copy link
Contributor Author

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

Tests/linting both pass.

AlanFoster added a commit that referenced this pull request Nov 7, 2015
@AlanFoster AlanFoster merged commit 4775969 into onefinestay:master Nov 7, 2015
@AlanFoster
Copy link
Member

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

@nnarhinen
Copy link

Can you push to NPM too?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants