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
Hooks API Discussion #1148
Comments
I can't wait to start using hooks in this library. As soon as the types are baked for the alpha we can set up a migration branch |
I'm playing around in a branch: experiment/hooks, just to think about how this API might look. The BoardSquare component is looking like this: const dropTarget = createDropTarget(ItemTypes.KNIGHT, {
canDrop: (props: BoardSquareProps) => canMoveKnight(props.x, props.y),
drop: (props: BoardSquareProps) => moveKnight(props.x, props.y),
})
const BoardSquare = ({ x, y, children }) => {
const black = (x + y) % 2 === 1
const [connectDropTarget, isOver, canDrop] = useDnd(
dropTarget,
connect => connect.dropTarget,
(connect, monitor) => !!monitor.isOver,
(connect, monitor) => !!monitor.canDrop,
)
return connectDropTarget(
<div>
<Square black={black}>{children}</Square>
{isOver && !canDrop && <Overlay color={'red'} />}
{!isOver && canDrop && <Overlay color={'yellow'} />}
{isOver && canDrop && <Overlay color={'green'} />}
</div>,
)
} So the idea here is that Note that this is just sketching up a candidate design, it's not actually implemented |
@darthtrevino What branch are you working on? I'm wondering if we can remove const dropTarget = createDropTarget(ItemTypes.KNIGHT, {
canDrop: (props: BoardSquareProps) => canMoveKnight(props.x, props.y),
drop: (props: BoardSquareProps) => moveKnight(props.x, props.y),
})
const BoardSquare = ({ x, y, children }) => {
const dropTargetElement = useRef(null);
const black = (x + y) % 2 === 1
const [isOver, canDrop] = useDnd(
dropTargetElement,
dropTarget,
...
)
return (
<div ref={dropTargetElement}>
<Square black={black}>{children}</Square>
{isOver && !canDrop && <Overlay color={'red'} />}
{!isOver && canDrop && <Overlay color={'yellow'} />}
{isOver && canDrop && <Overlay color={'green'} />}
</div>
)
} |
@jacobp100 I think I've seem similar hook-based APIs where the ref is provided and returned by the hook itself (like |
I suppose that works. It makes it harder to use the ref in multiple hooks, but there's nothing to stop you writing something that combines multiple refs into a single ref. What library was this? Guess we gotta see what the convention on this is! |
True, and true :-)
Can't seem find it again atm :-/ - lots of experiments around hooks in the past fortnight... There's https://github.com/beizhedenglong/react-hooks-lib that does But I do remember seeing some other API returning a ref straight from the hook. |
There's not much there, and it's not building at the moment, but the branch I'm in is |
Just iterating here: const dropTarget = createDropTarget(ItemTypes.KNIGHT, {
canDrop: props => canMoveKnight(props.x, props.y),
drop: props => moveKnight(props.x, props.y),
})
const BoardSquare = ({ x, y, children }) => {
const black = (x + y) % 2 === 1
const ref = useRef(null)
const [isOver, canDrop] = useDnd(
connect => connect(ref, dropTarget),
monitor => monitor.isOver,
monitor => monitor.canDrop,
)
return (
<div ref={ref}>
<Square black={black}>{children}</Square>
{isOver && !canDrop && <Overlay color={'red'} />}
{!isOver && canDrop && <Overlay color={'yellow'} />}
{isOver && canDrop && <Overlay color={'green'} />}
</div>,
)
} |
This is how chaining dragSource & dropTarget might work, if we use the ref as the first argument, the rest arguments can connect it to multiple dnd concepts. const dropTarget = createDropTarget(ItemTypes.CARD, {
canDrop: () => false
hover(props, monitor) {
/**/
},
})
const dragSource = createDragSource(ItemTypes.CARD, {
beginDrag: props => /*...*/,
endDrag: (props, monitor) => /*...*/
})
function Card({ text }) {
const ref = useRef(null)
const [isDragging] = useDnd(
connect => connect(ref, dropTarget, dragSource),
monitor => monitor.isDragging,
)
const opacity = isDragging ? 0 : 1
return (
<div ref={ref} style={{ ...style, opacity }}>
{text}
</div>
)
} |
So useDnd would look like export type DndConcept = DragSource | DropTarget
export type ConnectorFunction = (connector: Connector /*new type*/) => void
export type CollectorFunction<T> = (monitor: DragDropMonitor) => T
export function useDnd(
connector: ConnectorFunction,
...collectors: CollectorFunction[]
): any[] {
const dragDropManager = useDragDropManager()
// magic
return collectedProperties
}
export function useDragDropManager(): DragDropManager {
return useContext(DragDropContextType)
} |
Crazy idea, what if we didn't bother with const Test = props => {
const ref = useRef(null);
const { isDragging } = useDragSource(ref, ItemTypes.CARD, {
canDrag: props.enabled,
beginDrag: () => ({ id: props.id }),
});
return <div ref={ref} style={{ color: isDragging ? 'red' : 'black' }} />
} |
I think you can get rid of connect, but I'm not sure about monitor, it's where you get your props like isDragging from |
so maybe |
Can we just return all the props from monitor though? |
Maybe, I think this is the only method that would cause trouble:
IIRC, the DragSourceMonitor, DropTragetMonitor, and DragLayerMonitor all thunk down to the DragDropMonitor class. So I don't think we'd run into naming collisions, but I'd double check that. |
@yched Just playing around with this and hooks and noticed we do have to pass the ref in. See the case when something is both a source and a target, const Test = () => {
const ref = useRef(null)
const source = useDragSource(ref, …props)
const target = useDragTarget(ref, …props)
return <div ref={ref}>…</div>
} |
@jacobp100 indeed, makes sense. |
Okay, idea, const Test = (props) => {
const ref = useRef(null)
const sourceMonitor = useDragSource(ref, 'item' {
beginDrag: () => ({ id: props.id })
})
const targetMonitor = useDropTarget(ref, ['item'] {
drop: () => alert(targetMonitor.getItem().id)
})
const { isDragging } = useMonitorSubscription(targetMonitor, () => ({
isDragging: targetMonitor.isDragging()
})
return <div ref={ref}>…</div>
} Note that the specs for drag source and target don't receive any parameters, since you have access to them already
|
I've taken an initial look over here. Not got tests, but the chess example works with hooks - should show what I want to do! |
I think having Also having two fat objects next to each other is not a huge problem in my opinion because you had to do that before as well: const DragDrop = () => {
const ref = useRef(null);
const dragSource = {
beginDrag() {
return props;
}
};
const collectSource = monitor => {
return {
isDragging: monitor.isDragging()
};
};
const { isDragging } = useDragSource(ref, "Item", dragSource, collectSource);
const dropTarget = {
drop() {
return undefined;
}
};
const collectTarget = monitor => {
return {
isOver: monitor.isOver()
};
};
const { isOver } = useDropTarget(ref, "Item", dropTarget, collectTarget);
return <div ref={ref}>Drag me</div>;
}; The nice thing is that you can use values from other hooks as well. const Drag = () => {
const ref = useRef(null);
const context = useContext(Context)
const dragSource = {
beginDrag() {
context.setDragItem(props)
return props;
},
endDrag() {
context.setDragItem(null)
}
};
const collectSource = monitor => {
return {
isDragging: monitor.isDragging()
};
};
const { isDragging } = useDragSource(ref, "Item", dragSource, collectSource);
return <div ref={ref}>Drag me</div>;
}; One nice advantage is that we could remove props and component from the arguments beginDrag and all the other function accept as you already have access to them in the scope. |
^ I just updated my last comment to show that |
@jacobp100 I see. For me, the question would be if we need another hook to collect data from the monitor or if we could implement it in the useDragSource and useDropTarget as well. |
It made sense when stuff was HOCs, where you had to link up the connect stuff anyway. But there's now no technical requirement to couple them anymore, so I left them separate. Then you have more freedom with them - maybe one or more child components to respond to monitor changes, but not the component that is begin dragged. It also has an added bonus that if you aren't using monitor subscriptions, that hook can be tree shaken. That said, this is an initial draft! I'm not opposed to changing it if that's the consensus! |
That's a good argument. The one and only problem I see is that user could be confused when they call the monitor directly and wonder why it doesn't behave correctly: const Test = (props) => {
const ref = useRef(null)
const sourceMonitor = useDragSource(ref, 'item', {
beginDrag: () => ({ id: props.id })
})
const targetMonitor = useDropTarget(ref, ['item'], {
drop: () => alert(targetMonitor.getItem().id)
})
const { isDragging } = useMonitor(targetMonitor, () => ({
isDragging: targetMonitor.isDragging()
})
return <div ref={ref}>{sourceMonitor.isDragging() ? 'Dragging' : 'Drag me'}</div>
} Probably that can be solved with documentation and warnings when you call the function outside of the |
Actually, that will work! It's one of the things I'm not 100% keen on: the callback in Maybe something like this works better, const Test = (props) => {
...
useMonitorUpdates(targetMonitor, () => [targetMonitor.isDragging()]);
return <div ref={ref}>{sourceMonitor.isDragging() ? 'Dragging' : 'Drag me'}</div>
} Admittedly, it's much easier to introduce bugs with this form |
I am not 100% sure of the react-dnd internals but isn't the monitor there so we don't have to render the component everytime your mouse moves? So the previous would stop working if you remove the useMonitorSubscription and only have This would therefore not work correctly? const Test = (props) => {
const ref = useRef(null)
const sourceMonitor = useDragSource(ref, 'item', {
beginDrag: () => ({ id: props.id })
})
return <div ref={ref}>{sourceMonitor.isDragging() ? 'Dragging' : 'Drag me'}</div>
} |
The monitor has a Expanding on the previous post though, if we make optimising change detection an optional feature, this could be as simple as, const Test = (props) => {
...
useMonitorUpdates(sourceMonitor);
return <div ref={ref}>{sourceMonitor.isDragging() ? 'Dragging' : 'Drag me'}</div>
} |
A couple ideas. First, can we make the const dragSource = useDragSource('item', spec);
return <div ref={dragSource}/>
// or if you want to use a ref
const ref = useRef();
const dragSource = useDragSource('item', dragSourceSpec)(ref);
const dropTarget = useDropTarget('item', dropTargetSpec)(ref); Second, rather than making them invoke another hook in const dragSource = useDragSource('item', spec);
const { isDragging } = dragSource.subscribe(() => ({
isDragging: targetMonitor.isDragging()
})); |
I'm going to close this for now, since there's a candidate API. Feel free to comment on that with new issues though. Thanks! |
Looks like the hooks API has a flaw in the design: Swizec/useDimensions#3 |
Interesting, so I guess an alternative is that we use a connect function, like we currently do in the class-based API: const Box = ({ name }) => {
const [{ isDragging }, dragSource] = useDrag({
item: { name, type: ItemTypes.BOX },
end: (dropResult?: { name: string }) => {
if (dropResult) {
alert(`You dropped ${item.name} into ${dropResult.name}!`)
}
},
collect: monitor => ({
isDragging: monitor.isDragging(),
}),
})
const opacity = isDragging ? 0.4 : 1
return (
<div ref={node => dragSource(node)} style={{ ...style, opacity }}>
{name}
</div>
)
} So in general, the API would look like...
I was kind of hoping to move away from requiring the connector functions, but if the API is broken without them then we can make do |
As I read that issue though - our APIs are similar, but I think the problem there is that they're using a layout effect to get the measurement of a DOM node. We're not really using layout effects here, just registering DOM nodes with the dnd-core. |
@gaearon , our proposed hooks API is very similar to the useDimensions API - is that specific form an antipattern (e.g. |
@darthtrevino As far as I understand if you use the useDragSource hook and you pass the ref down to a child component and the child component rerenders we will not update the registered dom node in dnd-core: function Parent() {
const ref = useRef();
const dragSource = useDragSource(ref, ...);
return <Child connect={ref} />;
}
function Child({ connect }) {
const [open, setOpen] = useState(false);
function toggle() {
setOpen(!open);
}
return (
<Fragment>
<button onClick={toggle}>Toggle</button>
{open ? <div ref={connect} /> : null}
</Fragment>
);
} |
Yech. I'll see if I can make a test case for this later this week to prove whether it blows up or not |
If it blows up, using connector functions will be the fallback |
If I did everything correctly I was able to reproduce it here: https://codesandbox.io/s/xj7k9x4vp4 |
Kaboom, good work, thanks @k15a . I'll update the hooks API to use connect functions soon and include your example as a test case |
So I spent a bunch of time last night reworking the hooks API to use connector functions. As far as API design goes, I really fucking hate it. My next thought is that we can return a callback-ref instead of a ref-object. This should give us flexibility to use as a ref directly or to pass a ref into it Using Directly: let [props, dragSource] = useDrag({spec}) // dragSource result is a function
return <div ref={dragSource} {...props}>hey</div> Chaining let [dragProps, dragSource] = useDrag({spec})
let [dropProps, dropTarget] = useDrag({spec})
return <div ref={node => dragSource(dropTarget(node))}>hey</div> With Ref Object let ref = useRef(null)
let [dragProps, dragSource] = useDrag({spec})
let [dropProps, dropTarget] = useDrag({spec})
dragSource(dropTarget(ref))
return <div ref={ref}>hey</div> |
To me, it feels like I've created a ticket in the react repo. Feel free to comment. |
let [dragProps, dragSource] = useDrag({spec})
let [dropProps, dropTarget] = useDrag({spec})
return <div ref={node => dragSource(dropTarget(node))}>hey</div> I don't know how good this works as you would call the ref with every single render. So with every render, you would need to connect and disconnect. Also wouldn't it be better to have so node => {
dragSource(node)
dropTarget(node)
} |
It's would be the same thing |
So, to ease up on my earlier comment, the API in #1280 is shaping up to be better than I first thought. Feel free to comment here or there |
Somebody was going to ask eventually! The new hooks API could possibly help here. I think most of the API can pretty much stay the same, with the HOC mapping directly into a hook.
I do wonder if we can replace
connectDragSource
andconnectDropTarget
with just passing in the value ofuseRef
. It could definitely make stuff cleaner if that's possible!The text was updated successfully, but these errors were encountered: