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

feat(Transition): Only call findDOMNode if necessary #468

Closed
wants to merge 3 commits into from

Conversation

eps1lon
Copy link
Collaborator

@eps1lon eps1lon commented Mar 12, 2019

Another approach to #457. It only calls findDOMNode if any of the callbacks has the node argument specified.

This is only exploratory since seemingly unrelated implementation had to be change. Details at the affected line.

@@ -326,7 +326,7 @@ class Transition extends React.Component {
this.setNextCallback(handler)

const doesNotHaveTimeoutOrListener = timeout == null && !this.props.addEndListener
if (!node || doesNotHaveTimeoutOrListener) {
if (doesNotHaveTimeoutOrListener) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Has its own commit (87832e9). No test broke upon removing this so I need some guidance to understand this condition.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That PR did not introduce the evaluation of node.

@jquense
Copy link
Collaborator

jquense commented Mar 12, 2019

this is cool, the problem nowdays with this approach tho is destructuring messings with the observable arity of the function e.g.

function foo(...args) {
  console.log(args
}
foo.length // 0

@eps1lon
Copy link
Collaborator Author

eps1lon commented Mar 12, 2019

this is cool, the problem nowdays with this approach tho is destructuring messings with the observable arity of the function e.g.

function foo(...args) {
  console.log(args
}
foo.length // 0

It was to good to be true. This would be a breaking change then at which point we might as well remove the call completely.
Technical explanation that I missed:

This number excludes the rest parameter and only includes parameters before the first one with a default value.
-- https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Function/length#Description

Any thoughts about #468 (comment)?

@eps1lon eps1lon force-pushed the feat/unnecessary-findDOMNode branch from 02bee87 to c0d151d Compare April 9, 2019 19:06
eps1lon added a commit to eps1lon/react-transition-group that referenced this pull request Apr 11, 2019
Cherry-picking reactjs#468

## Breaking
Callbacks that use argument spread or access `arguments` will not have acces to the DOM node. The DOM node is only passed if Function.prototype.length is greater than 0.

```
// no node passed
const noNodeCallback = () => {};
// node passed
const withNodeCallback = node => {};
```

The first callback enables strict mode compatible usage of `Transition`. If you expect the node in callbacks a call to `findDOMNode` is required which issues a warning in `React.StrictMode`.
@eps1lon
Copy link
Collaborator Author

eps1lon commented Oct 14, 2021

Fixed with #559

@eps1lon eps1lon closed this Oct 14, 2021
@eps1lon eps1lon deleted the feat/unnecessary-findDOMNode branch October 14, 2021 13:58
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.

3 participants