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

Upgrade dependencies to fix build breakages #238

Merged
merged 2 commits into from
Dec 9, 2016
Merged

Upgrade dependencies to fix build breakages #238

merged 2 commits into from
Dec 9, 2016

Conversation

wincent
Copy link
Contributor

@wincent wincent commented Dec 9, 2016

We've had some lint-related build failures lately. Unfortunately, these masked test failures introduced by 35d8d38. When we fixed the lint (1eeb36c), the test failures showed up.

This commit fixes that by upgrading our deps:

  • Replace "react-addons-test-utils", which access a path that doesn't exist in current "react-dom", with "react-test-renderer".

This required some API updates in the tests, but also some changes to use ref-based DOM node access rather than ReactDOM.findDOMNode, which doesn't work in "react-test-renderer".

See:

Yea sorry, refs can work but findDOMNode() can't (we tried).

We've had some lint-related build failures:

  https://travis-ci.org/graphql/graphiql/builds/181453346

lately. Unfortunately, these masked test failures introduced by
35d8d38. When we fixed the lint (1eeb36c), the test failures showed
up:

  https://travis-ci.org/graphql/graphiql/builds/182431531

This commit fixes that by upgrading our deps:

- Replace "react-addons-test-utils", which access a path that doesn't
  exist in current "react-dom", with "react-test-renderer".

This required some API updates in the tests, but also some changes to
use ref-based DOM node access rather than `ReactDOM.findDOMNode`, which
doesn't work in "react-test-renderer".

See:

- facebook/react#7371
- facebook/react#8324

> Yea sorry, refs can work but `findDOMNode()` can't (we tried).
@wincent
Copy link
Contributor Author

wincent commented Dec 9, 2016

Test plan: updated a small app I have to use this build (with yarn link graphiql), hit http://localhost:3000/graphql and clicked around, ran some queries.

@wincent wincent requested a review from leebyron December 9, 2016 19:10
@@ -20,7 +17,7 @@ export default class CodeMirrorSizer {

updateSizes(components) {
components.forEach((component, i) => {
const size = ReactDOM.findDOMNode(component).clientHeight;
const size = component.getClientHeight();
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you check if DocExplorer also needs to expose this method?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think so. Only used from these.

This was referenced Dec 9, 2016
Copy link
Contributor

@leebyron leebyron left a comment

Choose a reason for hiding this comment

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

yisss

@leebyron leebyron merged commit f4f5783 into graphql:master Dec 9, 2016
@wincent wincent deleted the glh/fix-ci branch December 10, 2016 00:11
acao pushed a commit to acao/graphiql that referenced this pull request Jun 5, 2019
* Updated dependencies & released version v1.2
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants