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

Portal: rework #handleRef to avoid using ReactDOM.findDOMNode #1702

Closed
umamialex opened this issue May 24, 2017 · 2 comments
Closed

Portal: rework #handleRef to avoid using ReactDOM.findDOMNode #1702

umamialex opened this issue May 24, 2017 · 2 comments

Comments

@umamialex
Copy link

Relevant line: https://github.com/Semantic-Org/Semantic-UI-React/blob/master/src/addons/Portal/Portal.js#L417

ReactDOM.findDOMNode breaks ReactTestRenderer, and it will never be supported.

Additionally, even React documentation warns against using #findDOMNode.

Maybe each component could have a property nodeRef or similar that holds the DOM element? Then Portal could check to see if its ref is a ReactComponent or a ReactElement? The main downside is that custom components would need to add a nodeRef, which would be Semver Major.

Aside from actually solving the problem, my current workaround to keep Jest tests working is to add this to the top of the test:

jest.mock('react-dom', () => {
  return {
    findDOMNode: jest.fn(() => {})
  }
})

It might be useful to document this for people that do want to use Jest snapshot tests.

Steps

Add any Semantic UI component that uses Portal and try to use Jest snapshot testing.

Expected Result

Snapshot should be created and tests work as planned.

Actual Result

 FAIL  tests/app.jsx (6.945s)
  ● should check default app structure

    Invariant Violation: getNodeFromInstance: Invalid argument.

      at invariant (node_modules/fbjs/lib/invariant.js:44:15)
      at Object.getNodeFromInstance (node_modules/react-dom/lib/ReactDOMComponentTree.js:162:77)
      at Object.findDOMNode (node_modules/react-dom/lib/findDOMNode.js:49:41)
      at Portal._this.handleRef (node_modules/semantic-ui-react/dist/commonjs/addons/Portal/Portal.js:276:52)
      at attachRef (node_modules/react-test-renderer/lib/ReactRef.js:20:5)
      at Object.<anonymous>.ReactRef.attachRefs (node_modules/react-test-renderer/lib/ReactRef.js:42:5)
      at ReactCompositeComponentWrapper.attachRefs (node_modules/react-test-renderer/lib/ReactReconciler.js:23:12)
      at CallbackQueue.notifyAll (node_modules/react-test-renderer/lib/CallbackQueue.js:76:22)
      at ReactTestReconcileTransaction.close (node_modules/react-test-renderer/lib/ReactTestReconcileTransaction.js:36:26)
      at ReactTestReconcileTransaction.closeAll (node_modules/react-test-renderer/lib/Transaction.js:206:25)
      at ReactTestReconcileTransaction.perform (node_modules/react-test-renderer/lib/Transaction.js:153:16)
      at batchedMountComponentIntoNode (node_modules/react-test-renderer/lib/ReactTestMount.js:69:27)
      at ReactDefaultBatchingStrategyTransaction.perform (node_modules/react-test-renderer/lib/Transaction.js:140:20)
      at Object.batchedUpdates (node_modules/react-test-renderer/lib/ReactDefaultBatchingStrategy.js:62:26)
      at Object.batchedUpdates (node_modules/react-test-renderer/lib/ReactUpdates.js:97:27)
      at Object.render (node_modules/react-test-renderer/lib/ReactTestMount.js:125:18)
      at Object.<anonymous> (tests/app.jsx:15:41)
      at Promise.resolve.then.el (node_modules/p-map/index.js:42:16)
      at process._tickCallback (internal/process/next_tick.js:109:7)

Version

v0.68.3

@layershifter layershifter changed the title addons: Portal: Rework #handleRef to avoid using ReactDOM.findDOMNode. (Prevents Jest Snapshot testing) Portal: rework #handleRef to avoid using ReactDOM.findDOMNode May 25, 2017
@levithomason
Copy link
Member

levithomason commented May 25, 2017

Ideally, we'd remove findDOMNode usage entirely. My reasoning and proposed replacement are expressed here.

@layershifter
Copy link
Member

Portal now uses Ref to handle refs, see #2219. There is still findDOMNode under hood, but we can't get of it.

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

No branches or pull requests

3 participants