clauderic / react-sortable-hoc

A set of higher-order components to turn any list into an animated, accessible and touch-friendly sortable list✌️
https://clauderic.github.io/react-sortable-hoc/
MIT License
10.78k stars 977 forks source link

Omitted props fix is an API breakage #612

Open ianmstew opened 4 years ago

ianmstew commented 4 years ago

Hey all! Thanks for the awesome library.

I almost had a production issue on this, but fortunately caught it in testing. It seems that the props required by SortableElement HOC that previously passed through to the wrapper component are now intercepted and omitted. It also seems this was always the design, but it wasn't working until here: https://github.com/clauderic/react-sortable-hoc/compare/v1.10.0..v1.10.1#diff-2b4ca49d4bb0a774c4d4c1672d7aa781R23.

It just so happens that I was relying on index in a wrapped component and the side effect of no longer having index passed through was a showstopping breakage in my use case. This is not a complaint, I am just pointing out that this resulted in a breaking API change which was unexpected on a minor version bump, and I thought you'd appreciate a heads up.

ianmstew commented 4 years ago

It could be helpful as well to have use of a prop like disabled in the inner component should the inner component perhaps appear differently in that state. If there's a way to continue passing through all externally provided props, that would be ideal from my point of view. For now, I have to rename my props in order to make use of index and disabled.

abhijeetg007 commented 4 years ago

Totally agree with @ianmstew

nz-chris commented 4 years ago

Another "fix" is to pass through a prop props (I know, it's stupid). For example:

// Before
const SortableList = SortableContainer((props => (
    <div>
        {React.Children.map(props.children, (child, index) => {
            if (child) {
                if (!child.props.sortable) {
                    return child;
                }

                // props.useDragHandle is undefined!
                if (props.useDragHandle) {
                    return (
                        <SortableItemWithHandle
                            key={`item-${index}`}
                            index={index}
                            value={child}
                        />
                    )
                }

                return (
                    <SortableItem
                        key={`item-${index}`}
                        index={index}
                        value={child}
                    />
                );
            }
        })}
    </div>
));

class SortableHOC extends React.Component {
    render() {
        const {
            useDragHandle,
            children,
        } = this.props;

        return (
            <SortableList
                useDragHandle={useDragHandle}
            >
                {children}
            </SortableList>
        );
    }
} 
// After. Changes surrounded in **.
const SortableList = SortableContainer((**({ props })** => (
    <div>
        {React.Children.map(props.children, (child, index) => {
            if (child) {
                if (!child.props.sortable) {
                    return child;
                }

                // props.useDragHandle is **DEFINED!!!**
                if (props.useDragHandle) {
                    return (
                        <SortableItemWithHandle
                            key={`item-${index}`}
                            index={index}
                            value={child}
                        />
                    )
                }

                return (
                    <SortableItem
                        key={`item-${index}`}
                        index={index}
                        value={child}
                    />
                );
            }
        })}
    </div>
));

class SortableHOC extends React.Component {
    render() {
        const {
            useDragHandle,
            children,
        } = this.props;

        return (
            <SortableList
                useDragHandle={useDragHandle}
                **props={this.props}**
            >
                {children}
            </SortableList>
        );
    }
}