Skip to content
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

[BUG] - DropdownMenu component onSelectionChange type not correct. #2512

Open
AlbertoEE opened this issue Mar 12, 2024 · 10 comments · May be fixed by #2567
Open

[BUG] - DropdownMenu component onSelectionChange type not correct. #2512

AlbertoEE opened this issue Mar 12, 2024 · 10 comments · May be fixed by #2567
Labels
🐛 Type: Bug Something isn't working

Comments

@AlbertoEE
Copy link

NextUI Version

2.2.9

Describe the bug

Context

The <DropdownMenu /> component coming from the nextui-org/dropdown package has these 2 props:

  • onSelectionChange
  • selectedKeys

If you take a look at the typescript signature of the props you will see that:

export type Selection = 'all' | Set<Key>;

export interface MultipleSelection {
  /** The type of selection that is allowed in the collection. */
  selectionMode?: SelectionMode,
  /** Whether the collection allows empty selection. */
  disallowEmptySelection?: boolean,
  /** The currently selected keys in the collection (controlled). */
  selectedKeys?: 'all' | Iterable<Key>,
  /** The initial selected keys in the collection (uncontrolled). */
  defaultSelectedKeys?: 'all' | Iterable<Key>,
  /** Handler that is called when the selection changes. */
  onSelectionChange?: (keys: Selection) => any,
  /** The currently disabled keys in the collection (controlled). */
  disabledKeys?: Iterable<Key>
}

The problem

In reality if you make use of these two props and debug the insides of the parameter that is passed to the onSelectionChange callback, you will see that the object has this structure.

{
  anchorKey: "foo",
  currentKey: "foo",
}

Taking a look at the constructor in the debugger we can see that the type that is being returned in reality is:

class ... extends Set {
    constructor(keys, anchorKey, currentKey){
        super(keys);
        if (keys instanceof ...) {
            this.anchorKey = anchorKey || keys.anchorKey;
            this.currentKey = currentKey || keys.currentKey;
        } else {
            this.anchorKey = anchorKey;
            this.currentKey = currentKey;
        }
    }
}

After investigating a little bit I found that this type is coming from the package ``@react-stately/selection/src/Selection.ts`:

/*
 * Copyright 2020 Adobe. All rights reserved.
 * This file is licensed to you under the Apache License, Version 2.0 (the "License");
 * you may not use this file except in compliance with the License. You may obtain a copy
 * of the License at http://www.apache.org/licenses/LICENSE-2.0
 *
 * Unless required by applicable law or agreed to in writing, software distributed under
 * the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR REPRESENTATIONS
 * OF ANY KIND, either express or implied. See the License for the specific language
 * governing permissions and limitations under the License.
 */

import {Key} from '@react-types/shared';

/**
 * A Selection is a special Set containing Keys, which also has an anchor
 * and current selected key for use when range selecting.
 */
export class Selection extends Set<Key> {
  anchorKey: Key;
  currentKey: Key;

  constructor(keys?: Iterable<Key> | Selection, anchorKey?: Key, currentKey?: Key) {
    super(keys);
    if (keys instanceof Selection) {
      this.anchorKey = anchorKey || keys.anchorKey;
      this.currentKey = currentKey || keys.currentKey;
    } else {
      this.anchorKey = anchorKey;
      this.currentKey = currentKey;
    }
  }
}

Maybe...

Could it be that the DropDown component is using this Selection from @react-stately type instead of the one shown in the signature (export type Selection = 'all' | Set<Key>)?

I tried to look at the repo code but I'm new to Frontend in general and it was really hard to read for me so I couldn't find where to fix it so I could create a PR.

Your Example Website or App

No response

Steps to Reproduce the Bug or Issue

Just use a DropdownMenu like this one and debug the value that is being passed to the onSelectionChange callback.

<Dropdown>
        <DropdownTrigger>
          <div>
            Test
          </div>
        </DropdownTrigger>
        <DropdownMenu
          aria-label="Single selection example"
          variant="flat"
          disallowEmptySelection
          selectionMode="single"
          selectedKeys={selectedKeys}
          onSelectionChange={handleOnDropdownChange}
        >
          {generateList()}
        </DropdownMenu>
</Dropdown>

Expected behavior

I would expect to get the type export type Selection = 'all' | Set<Key>; not the type coming from @react-stately/selection/src/Selection.ts

Screenshots or Videos

No response

Operating System Version

Windows 10 pro

Browser

Chrome

@AlbertoEE AlbertoEE added the 🐛 Type: Bug Something isn't working label Mar 12, 2024
Copy link

linear bot commented Mar 12, 2024

@simPod
Copy link

simPod commented Mar 14, 2024

Same applies for Select component.

@rojuvi
Copy link

rojuvi commented Mar 14, 2024

It happens

@winchesHe winchesHe linked a pull request Mar 22, 2024 that will close this issue
@winchesHe
Copy link
Member

winchesHe commented Mar 22, 2024

@AlbertoEE @simPod maybe this pr can resolve your problem ? #2567

@AlbertoEE
Copy link
Author

AlbertoEE commented Mar 22, 2024

@winchesHe I think that the PR does not fix the problem as it looks like that type is not the intended one that should be used instead only export type Selection = 'all' | Set<Key>; should be used.

Maybe I'm wrong but that is my gut feeling.

@winchesHe
Copy link
Member

@AlbertoEE You can console.log the keys value, It was Set<React.Key> & {anchorKey?: string; currentKey?: string}, or what type do you think it should be
image

@sutter
Copy link

sutter commented Mar 27, 2024

Same problem here

@ameytessact
Copy link

Similar issue maybe:

  const [value, setValue] = useState<Selection>(new Set([]));
Argument of type 'Set<never>' is not assignable to parameter of type 'Selection | (() => Selection)'.ts(2345)

@winchesHe
Copy link
Member

Similar issue maybe: 类似的问题可能是:

  const [value, setValue] = useState<Selection>(new Set([]));
Argument of type 'Set<never>' is not assignable to parameter of type 'Selection | (() => Selection)'.ts(2345)

Change like following

  const [value, setValue] = useState<Set<React.Key>>(new Set([]));

@ameytessact
Copy link

@winchesHe

const [openedWorkspaces, setOpenedWorkspaces] = useState<Set<Key>>(new Set([]));
<Accordion
  selectedKeys={openedWorkspaces}
  onSelectionChange={setOpenedWorkspaces}
>

hi, I still get the following error

Type 'Dispatch<SetStateAction<Set<Key>>>' is not assignable to type '(keys: Selection) => any'.
  Types of parameters 'value' and 'keys' are incompatible.
    Type 'Selection' is not assignable to type 'SetStateAction<Set<Key>>'.
      Type 'string' is not assignable to type 'SetStateAction<Set<Key>>'.ts(2322)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐛 Type: Bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants