Fix typings for DropTarget and DragSource decorators

[jeanfortheweb]

The decorator function has to take a component accepting both, props and collected props - but it should return a component which only takes the own props

[jeanfortheweb]

Same goes now for the DragSource decorator

[lukyth]

Is there anything I can help to get this merged? I also face this issue and really want it to be solved.

[lukyth]

TargetClass can't be omitted in the returned value because properties in that class would be lost. So I think it should be:

return function decorateSource<TargetClass extends React.ComponentType<Props & CollectedProps>>(DecoratedComponent: TargetClass): TargetClass & DndComponentClass<Props> {

I've tested this in my own project and everything work fine. TBH, I'm not sure why I don't have to pass CollectedProps to the result of this 😅

The code below doesn't give any type error. Notice that I don't have to pass connectDragSource and isDragging when I use ProjectBoxDragContainer. I also don't lose type of ProjectBoxDragContainer.test.

const projectSource = {
  beginDrag(props: IProjectBoxProps) {
    return { projectId: props.projectId }
  },
}

function collect(connect: DragSourceConnector, monitor: DragSourceMonitor) {
  return {
    connectDragSource: connect.dragSource(),
    isDragging: monitor.isDragging(),
  }
}

class ProjectBoxDragContainer extends React.PureComponent<{
  isDragging: boolean
  connectDragSource: ConnectDragSource
} & IProjectBoxProps> {
  public static test = '1'
  public render() {
    return this.props.connectDragSource(
      <div style={{ opacity: this.props.isDragging ? 0 : 1 }}>
        <ProjectBox width={this.props.width} projectId={this.props.projectId} />
      </div>
    )
  }
}

export default DragSource('project', projectSource, collect)(ProjectBoxDragContainer)

// Usage
<ProjectBoxDragContainer
  width={ProjectBoxDragContainer.test}
  projectId={p.id}
/>

[darthtrevino]

Taking a look

[darthtrevino]

@jeanfortheweb @lukyth Can you guys check out the latest version on this branch? I think the types make sense, but I'd like your input

edit: ugh, types

[lukyth]

Tested and everything works fine. Just one more comment which is totally optional.

[lukyth]

How about instead of casting type here, we make DecoratedComponent in decorateHandler to be TargetClass? i.e. lukyth@b101a83

[darthtrevino]

Are you using Functions instead of Decorators in your code? I can make this work with using DragSource and DropTarget as functions, but the types with Decorators gets funky.

To make the API work like @jeanfortheweb wants, we need to exclude props that may be collected from the component interface, so the return type will be a React.ComponentType, excluding collected ones.

That's nice for clients, and it cleans up some concerns about null checking since we can put the collected props in a more rigid interface without optional attributes.

The problem comes when we try to use decorators, Decorators need the original type information to be part of the resultant type (at least that's what I gather from the errors I'm getting).

So if we make this change, it will break types for people who use DragSource and DropTarget as decorators. Not super ideal.

I'd be happy to look at other suggestions on fixing this, but the examples need to compile as-is with decorators for now.

Now, keep in mind that when React 16.7 drops, we will probably rethink this API entirely with hooks, and eliminate the decorator component pattern.

[lukyth]

I just notice the problem with decorator since I'm always using it as a function. I tried many ways and nothing works. With the current TypeScript (at least while Request: Class Decorator Mutation issue is still open), I don't think we can solve this omitting props issue in decorator, or even solving it only for the function usage without breaking the decorator.

So.. maybe we have to live with this for now? 😞

[darthtrevino]

Maybe - one option could be to discontinue decorator support in TS. The API would work the same in JS, but you'd need to use function-based decorators for strong typings.

But honestly, I think it would make more sense to think about how the hooks-based API will look instead.

[lukyth]

Then for this PR, can we change the example code to use function-based decorators and make the type definition omit collected props in that use case? The hooks-based API will probably be better to discuss in its own issue.

[darthtrevino]

I agree. I migrated the doc site last night to Gatsby, and the examples were changed to remove decorators. Decorators have been de-emphasized in our documentation, but still are usable in JavaScript. The doc site needed updating anyway because the es5 examples are well deprecated now and es7 is technically an incorrect term for the decorator proposal After thinking about it, given the choice between correct types or decorators working in TS, I think we should lean towards correct types. I'll cut a major and explain our justification in the release notes

…

On Tue, Nov 6, 2018, 12:40 AM Kanitkorn Sujautra ***@***.*** wrote: ***@***.**** commented on this pull request. ------------------------------ In packages/react-dnd/src/DragSource.ts <#1134 (comment)>: > >(DecoratedComponent: TargetClass): TargetClass & DndComponentClass<Props> { return decorateHandler<Props, TargetClass, SourceType>({ containerDisplayName: 'DragSource', createHandler: createSource, registerHandler: registerSource, createMonitor: createSourceMonitor, createConnector: createSourceConnector, - DecoratedComponent, + DecoratedComponent: (DecoratedComponent as any) as React.ComponentType< + Props + >, Then for this PR, can we change the example code to use function-based decorators and make the type definition omit collected props in that use case? The hooks-based API will probably be better to discuss in its own issue. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#1134 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAG7iNnJmYNNto7xeHwIXF6xYJZXvnZvks5usUsTgaJpZM4WslCl> .

[lukyth]

I was planned to help you with the example migration at first but you already got it all done 😮 That's a whole lot of work you did there. Thank you so much for your afford! 🙇