Added findDOMNode, as we move toward deprecating g #2646
Conversation
|
Nevermind me... or? |
|
The bigger thing is that we're moving it off the component and into a static helper/utility function. That's the structural change, the name change is more of an "oh, while we're making the api change here, let's fix the name also, so users don't need to do two migrations" I don't remember all the reasons we didn't like "get", but I think one of them might have been related to the fact that we may not have a 1-to-1 mapping from react components to dom nodes ([might be 1-to-0 today, might be extended to 1-to-n in future] - maybe I just made that up), and "get" has a connotation of being an instance method with a 1-to-1 mapping. Regardless, I know we were talking about "queryDOMNode" and "selectDOMNode", both of which were too easily confused with jquery selectors. Sema indicated we should go with "findDOMNode" |
|
@syranide no, you didn't miss any explicit discussion. This is part of the decoupling that @sebmarkbage has been working on but some of the details haven't been well communicated. Specifically in this case, I'm not entirely happy with naming so we could probably bikeshed some more if you want. I'm personally leaning towards |
| if (isDomElement(element)) { | ||
| return element; | ||
| } | ||
| invariant(false, 'Assert: NOT REACHED (keys: '+Object.keys(element)+')'); |
There was a problem hiding this comment.
We'll want a better message here. Also, we need to use printf format (with only %s), not string concat. invariant(condition, 'some %s string %s', 1, 2)
jimfb
force-pushed
the
getDOMNode-becomes-findDOMNode
branch
3 times, most recently
from
e419094 to
414717c
Compare
| return element; | ||
| } | ||
| invariant(false, | ||
| 'Element appears to be neither ReactComponent nor DOMNode (keys: %s)', |
jimfb
force-pushed
the
getDOMNode-becomes-findDOMNode
branch
from
414717c to
1d55163
Compare
jimfb
force-pushed
the
getDOMNode-becomes-findDOMNode
branch
2 times, most recently
from
fe77ef8 to
0308e03
Compare
|
This should move into the browser folder since it's DOM specific. |
| return ReactMount.getNodeFromInstance(componentOrElement); | ||
| } | ||
| invariant( | ||
| !(componentOrElement.render != null && typeof(componentOrElement) === 'function'), |
There was a problem hiding this comment.
I think you meant to check typeof componentOrElement.render === 'function', not the componentOrElement itself. Please add a unit test that covers this error message case.
jimfb
force-pushed
the
getDOMNode-becomes-findDOMNode
branch
3 times, most recently
from
903f056 to
0810b08
Compare
jimfb
force-pushed
the
getDOMNode-becomes-findDOMNode
branch
from
0810b08 to
8bbc60c
Compare
jimfb
force-pushed
the
getDOMNode-becomes-findDOMNode
branch
from
8bbc60c to
b46a6ce
Compare
|
👍 |
Added findDOMNode, as we move toward deprecating getDOMNode
| invariant( | ||
| !(componentOrElement.render != null && typeof(componentOrElement.render) === 'function'), | ||
| 'Component contains `render` method but is not mounted in the DOM', | ||
| Object.keys(componentOrElement) |
There was a problem hiding this comment.
There is no %s so these won't be inserted anywhere.
There was a problem hiding this comment.
Excellent catch, thanks!
|
Updated diff: #2800 |
Summary: Currently Forget bails on mutations to globals within any callback function. However, callbacks passed to useEffect should not bail and are not subject to the rules of react in the same way. We allow this by instead of immediately raising errors when we see illegal writes, storing the error as part of the function. When the function is called, or passed to a position that could call it during rendering, we bail as before; but if it's passed to `useEffect`, we don't raise the errors.
Added findDOMNode, as we move toward deprecating getDOMNode