I was wondering if it's a good idea to pass the refs to getDerivedStateFromProps. I understand why passing the instance is not welcomed, but there are some edge cases where the next state is calculated based on refs.
Take a look at the example below:
const getFlyoutPosition = ({
idealDirection,
margin = parseInt(staticTooltip.margin, 10),
relativeOffset,
triggerRect
}) => {
const windowSize = {
height: window.innerHeight,
width: window.innerWidth,
scrollX: 0,
scrollY: 0
};
// Here `this.flyoutRef` is needed to get the element dimensions and use them to calculate the next state
// At the moment there is no way to access the refs in `getDerivedStateFromProps`
const flyoutSize = {
height: this.flyoutRef ? this.flyoutRef.clientHeight : 0,
width: this.flyoutRef ? this.flyoutRef.clientWidth : 0
};
const mainDir = getMainDir(flyoutSize, idealDirection, triggerRect, windowSize, margin);
const base = baseOffsets(relativeOffset, flyoutSize, mainDir, triggerRect, windowSize, margin);
const flyoutOffset = {
top: base.top,
left: base.left
};
return { flyoutOffset, mainDir };
};
export default class extends Component {
state = {
flyoutOffset: {
top: undefined,
right: undefined,
bottom: undefined,
left: undefined
},
mainDir: null,
};
static getDerivedStateFromProps = getFlyoutPosition
setFlyoutRef = (c) => { this.flyoutRef = c; }
flyoutRef;
render() {
const { flyoutOffset } = this.state;
const { children } = this.props;
return (
<div
ref={this.setFlyoutRef}
style={{ position: 'absolute', ...flyoutOffset }}
>
{children}
</div>
);
}
}
I think we should consider passing refs to getDerivedStateFromProps as sometimes the state may be based on values from the DOM.
Please let me know if I should create a PR.
I would be interested in hearing how others have handled this case. We have ended up storing the refs in state so we could access them during getDerivedStateFromProps. In my use case the value in state needs to be updated before any changes are flushed to the dom.
The "correct" way to do this would be to move the state update to componentDidUpdate, if you need to take measurements before the render, getSnapshotBeforeUpdate is exactly the right tool for the job here
It's not clear to me why your code is both _reading values from_ and _writing values to_ a ref in response to a change in external props. I don't think there's enough context in the code example above to really understand why that's important (e.g. what's mainDir used for and how is getMainDir related to your component).
But I suspect you shouldn't need to use getDerivedStateFromProps for that. In general, there are usually better solutions than getDerivedStateFromProps.
Jason mentioned the getSnapshotBeforeUpdate lifecycle, but I'm not sure that's really needed in this case either.
From my limited knowledge of this component, I would be inclined to say that you could just do everything you're doing in componentDidUpdate. Something like this:
function updateFlyoutPosition(props, flyout) {
if (flyout) {
const windowSize = {
height: window.innerHeight,
width: window.innerWidth,
scrollX: 0,
scrollY: 0
};
const flyoutSize = {
height: flyout.clientHeight,
width: flyout.clientWidth
};
const mainDir = getMainDir(
flyoutSize,
idealDirection,
triggerRect,
windowSize,
margin
);
const base = baseOffsets(
relativeOffset,
flyoutSize,
mainDir,
triggerRect,
windowSize,
margin
);
// Adjust position of the flyout
flyout.style.top = `${base.top}px`;
flyout.style.left = `${base.left}px`;
// I don't know what mainDir is used for you.
// Your example didn't show.
}
}
export default class extends React.Component {
flyoutRef = React.createRef();
componentDidMount() {
// Always update the position of the flyout after the first render
updateFlyoutPosition(this.props, this.flyoutRef.current);
}
componentDidUpdate(prevProps, prevState) {
// Only update the position of the flyout if related props have changed
if (
this.props.idealDirection !== prevProps.idealDirection ||
this.props.margin !== prevProps.margin ||
this.props.relativeOffset !== prevProps.relativeOffset ||
this.props.triggerRect !== prevProps.triggerRect
) {
updateFlyoutPosition(this.props, this.flyoutRef.current);
}
}
render() {
const { children } = this.props;
return (
<div ref={this.flyoutRef} style={{ position: "absolute" }}>
{children}
</div>
);
}
}
I would be interested in hearing how others have handled this case. We have ended up storing the refs in state so we could access them during
getDerivedStateFromProps. In my use case the value in state needs to be updated before any changes are flushed to the dom.
When do you call setState with the ref? For what I know, it is a bad practice to call setState during render, so I don't think that approach is the correct one.
The "correct" way to do this would be to move the state update to
componentDidUpdate, if you need to take measurements before the render,getSnapshotBeforeUpdateis exactly the right tool for the job here
I have tried using componentDidUpdate with getSnapshotBeforeUpdate and it didn't work quite well. componentDidUpdate is triggered after the component receives the props and renders, which is too late.
It's not clear to me why your code is both _reading values from_ and _writing values to_ a ref in response to a change in external props. I don't think there's enough context in the code example above to really understand why that's important (e.g. what's
mainDirused for and how isgetMainDirrelated to your component).But I suspect you shouldn't need to use
getDerivedStateFromPropsfor that. In general, there are usually better solutions thangetDerivedStateFromProps.Jason mentioned the
getSnapshotBeforeUpdatelifecycle, but I'm not sure that's really needed in this case either.From my limited knowledge of this component, I would be inclined to say that you could just do everything you're doing in
componentDidUpdate. Something like this:function updateFlyoutPosition(props, flyout) { if (flyout) { const windowSize = { height: window.innerHeight, width: window.innerWidth, scrollX: 0, scrollY: 0 }; const flyoutSize = { height: flyout.clientHeight, width: flyout.clientWidth }; const mainDir = getMainDir( flyoutSize, idealDirection, triggerRect, windowSize, margin ); const base = baseOffsets( relativeOffset, flyoutSize, mainDir, triggerRect, windowSize, margin ); // Adjust position of the flyout flyout.style.top = `${base.top}px`; flyout.style.left = `${base.left}px`; // I don't know what mainDir is used for you. // Your example didn't show. } } export default class extends React.Component { flyoutRef = React.createRef(); componentDidMount() { // Always update the position of the flyout after the first render updateFlyoutPosition(this.props, this.flyoutRef.current); } componentDidUpdate(prevProps, prevState) { // Only update the position of the flyout if related props have changed if ( this.props.idealDirection !== prevProps.idealDirection || this.props.margin !== prevProps.margin || this.props.relativeOffset !== prevProps.relativeOffset || this.props.triggerRect !== prevProps.triggerRect ) { updateFlyoutPosition(this.props, this.flyoutRef.current); } } render() { const { children } = this.props; return ( <div ref={this.flyoutRef} style={{ position: "absolute" }}> {children} </div> ); } }
The component is reading the 'div' sizes, and does some calculations to see if it fits on the screen, then writes the position values to update the 'div'. The 'div' is positioned relatively to another element, an 'anchor', so it looks when the position or size of the 'anchor' changes, then updates the flyout element.
I do like your approach, I have tried writing the styles to the ref directly before opening the issue, but I wasn't sure if I should've written the styles myself or leave it to react, as it decides better when to update the DOM.
Sorry for not posting enough of the code, here is what 'getMainDir' does:
export const curryTernary = predicate => right => left => (predicate ? right : left);
export const setOrReset = predicate => right => curryTernary(predicate)(right)([0, 0]);
export const getMainDir = (flyoutSize, idealDirection, triggerRect, windowSize, margin = 0) => {
const predicateVertical = windowSize.width - triggerRect.right > 0;
const predicateHorizontal = windowSize.height - triggerRect.bottom > 0;
const [up, down] = setOrReset(predicateVertical)([
(triggerRect.top - flyoutSize.height) + margin,
windowSize.height - flyoutSize.height - triggerRect.bottom - margin
]);
const [left, right] = setOrReset(predicateHorizontal)([
(triggerRect.left - flyoutSize.width) - margin,
windowSize.width - flyoutSize.width - triggerRect.right - ERROR_OFFSET - margin
]);
const spaces = [up, right, down, left];
const max = Math.max(...spaces);
return idealDirection && spaces[parseInt(DIRECTIONS_INDEX_MAP[idealDirection], 10)] > 0
? idealDirection
: SPACES_INDEX_MAP[spaces.indexOf(max)];
};
It basically returns one of the values: 'up', 'down', 'left', 'right' which will be the direction of the flyout.
Here's the other function:
export const baseOffsets = (relativeOffset, flyoutSize, mainDir, triggerRect, windowSize, margin = 0) => {
let top = windowSize.scrollY + triggerRect.top;
let left = windowSize.scrollX + triggerRect.left;
if (mainDir === DIRECTIONS.DOWN) {
top = windowSize.scrollY + triggerRect.bottom + margin;
} else if (mainDir === DIRECTIONS.UP) {
top = (windowSize.scrollY + triggerRect.top) - flyoutSize.height - margin;
}
if (mainDir === DIRECTIONS.LEFT) {
left = (windowSize.scrollX + triggerRect.left) - flyoutSize.width - margin;
} else if (mainDir === DIRECTIONS.RIGHT) {
left = windowSize.scrollX + triggerRect.right + margin;
}
return {
top: top - relativeOffset.y,
left: left - relativeOffset.x
};
};
@NulledGravity I think my use case is basically the same as yours 馃槅, we are doing this inside of a positioner component that determines the top/left position values for a popover/tooltip.
We did run into some issues with storing refs in state causing cascading updates, but somehow this code got us around that issue:
class Tooltip extends React.Component {
// some code omitted
hasMounted = false; // set to true in componentDidMount
assignRef = ref => {
if (this.hasMounted && ref !== null) {
this.setState({ target: ref });
}
}
}
An alternative approach we considered was to store the refs on the instance and then setState once they are stored (i.e. deferred from the initial render pass) but ran into some issues doing that.
I think the route we will end up going is two pass rendering (Brian's suggestion above), updating in componentDidUpdate with the positioned element setting visibility: hidden in its styles, and then setting state to update the position and finally setting visibility: visible on the tooltip. It's unfortunate that it needs to be in two passes, but those passes are generally quite fast.
Hey @hamlim, those are interesting approaches 馃槃.
I used Brian's suggestion with small changes to fit my needs, but in the end I am updating the DOM directly with this.flyoutRef.style.top in componentDidMount and componentDidUpdate.
Most helpful comment
It's not clear to me why your code is both _reading values from_ and _writing values to_ a ref in response to a change in external props. I don't think there's enough context in the code example above to really understand why that's important (e.g. what's
mainDirused for and how isgetMainDirrelated to your component).But I suspect you shouldn't need to use
getDerivedStateFromPropsfor that. In general, there are usually better solutions thangetDerivedStateFromProps.Jason mentioned the
getSnapshotBeforeUpdatelifecycle, but I'm not sure that's really needed in this case either.From my limited knowledge of this component, I would be inclined to say that you could just do everything you're doing in
componentDidUpdate. Something like this: