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

[breaking change] Remove findDOMNode #1285

Merged
merged 9 commits into from
Nov 6, 2018

Conversation

chandlerprall
Copy link
Contributor

@chandlerprall chandlerprall commented Nov 2, 2018

Summary

Closes #1261

Removes all uses of ReactDOM.findDOMNode from EUI.

Breaking Changes

  • EuiMutationObserver changed to a render prop, passing a ref callback to the render function
  • EuiPortal no longer accepts a React node for insert.sibling value
  • popover positioning service's methods no longer accept React node values

Non-Breaking Changes

  • EuiAccordion's use of EuiMutationObserver updated
  • EuiPopover's use of EuiMutationObserver updated
  • EuiToolTip's use of EuiMutationObserver updated
  • EuiWrappingPopover removed internal use of findDOMNode

Checklist

- [ ] This was checked in mobile

  • This was checked in IE11
    - [ ] This was checked in dark mode
    - [ ] Any props added have proper autodocs
    - [ ] Documentation examples were added
  • A changelog entry exists and is marked appropriately
  • This was checked for breaking changes and labeled appropriately
  • Jest tests were updated or added to match the most common scenarios
  • This was checked against keyboard-only and screenreader scenarios
    - [ ] This required updates to Framer X components

@chandlerprall
Copy link
Contributor Author

I'm holding off on adding changelog / wanting to merge until Monday

Copy link
Member

@weltenwort weltenwort left a comment

Choose a reason for hiding this comment

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

Nice, I like it that the APIs are less overloaded 👍 Left just a few inline comments below...

this.updateChildNode();
if (this.childNode == null) {
throw new Error('EuiMutationObserver did not receive a ref');
}
Copy link
Member

Choose a reason for hiding this comment

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

I wonder why we need this? What's wrong with registering a ref later, e.g. when rendering conditionally?

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 feel too strongly here, but my thoughts on requiring a ref from the beginning:

  • when possible, error earlier and faster for better dev experience
  • MutationObserver is inherently an invisible thing, it would be trivial to forget to forward the ref or do so incorrectly and assume things are working as expected with no feedback
  • if conditional rendering is required, it's trivial to wrap EuiMutationObserver instead

Copy link
Member

Choose a reason for hiding this comment

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

This conditional means the component only works if all child component mount operations are flushed to the DOM and ref callbacks are called before the componentDidMount of this component is called. Is that always guaranteed to be the case?

We might be relying too much on the mounting behavior across multiple components here, which is not documented in that detail to my knowledge. For example, the behavior might change with lazy rendering in newer React version.

It also artificially restricts a capability the consumer might expect from a "normal" react component (i.e. the ability to change refs at any point during the lifecycle).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is that always guaranteed to be the case?

Yes, React provides that guarantee to support all DOM-related side effectives in didMount. You're right that lazy rendering could interfere here, but I think that goes back to if conditional rendering is required, it's trivial to wrap EuiMutationObserver instead.

It also artificially restricts a capability the consumer might expect from a "normal" react component (i.e. the ability to change refs at any point during the lifecycle).

The updateChildNode method supports that ref changing at any point. This error simply enforces that ref being set on the first render pass.

src/components/mutation_observer/mutation_observer.js Outdated Show resolved Hide resolved
src/components/portal/portal.js Show resolved Hide resolved
Copy link
Member

@weltenwort weltenwort left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@chandlerprall chandlerprall merged commit 23f3058 into elastic:master Nov 6, 2018
@chandlerprall chandlerprall deleted the remove-finddomnode branch November 6, 2018 19:00
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.

2 participants