overloaded the open function for convenient type inference

[Fractal-Tess]

What kind of change does this PR introduce?

• Bugfix
• Feature
• Docs
• New Binding issue #___
• Code style update
• Refactor
• Build-related changes
• Other, please describe:

Does this PR introduce a breaking change?

• Yes, and the changes were approved in issue #___
• No

Checklist

• When resolving issues, they are referenced in the PR's title (e.g fix: remove a typo, closes #___, #___)
• A change file is added if any packages will require a version bump due to this PR per the instructions in the readme.
• I have added a convincing reason for adding this feature, if necessary

Other information

This pr introduces a simple function overload to provide convenient types based on input options.
I've refactored the original examples, cutting one of them outright and reducing the first one.
However, if you feel like this is not enough, please request a change with some instructions on how to make it better.

[lucasfernog]

Duplicate of #5128. I'm not sure if this is a breaking change (like if you're using { multiple: false } and checking if the response is an array).

[FabianLars]

The duplicate is my bad, forgot that 5128 exists.

i don't think this is a breaking change btw. I can't think of any scenario and nothing occurred in my testing (i tried to explicitly test this which is also where we noticed the issue that was fixed in the last commit).
So the only situation where it would be "breaking" is when the user's code is buggy, for example when they set multiple: false but use the return value as if it would be an array, in which case this would be a good "breaking" change imho.

But yeah i see why you'd prefer to be safe here, with JS/TS being what it is there are probably 10 edge cases here i could never even think of 😂

[jianxingxuejian]

@lucasfernog I think function overload is more readable than conditional types, maybe I should close my pr #5128 ?
Still, I think this type is a bit wrong, it should look like this:

async function open(
  options: OpenDialogOptions & { multiple: true }
): Promise<null | string[]>
async function open(options?: OpenDialogOptions): Promise<null | string>
async function open(
  options?: OpenDialogOptions
): Promise<null | string[] | string> {}

const test1 = await open() // string | null
const test2 = await open({}) // string | null
const test3 = await open({ multiple: true }) // string[] | null
const test4 = await open({ multiple: false }) // string | null

Further, should the options be restricted entirely to the OpenDialogOptions type?

async function open<T extends OpenDialogOptions = OpenDialogOptions>(
  options: T & { multiple: true }
): Promise<null | string[]>
async function open<T extends OpenDialogOptions = OpenDialogOptions>(
  options?: T
): Promise<null | string>
async function open<T extends OpenDialogOptions = OpenDialogOptions>(
  options?: T
): Promise<null | string[] | string> {}

interface MyOptions extends OpenDialogOptions {
  other: any
}

const test5 = await open({ multiple: true, other: 'test' }) // OK
const test6 = await open<MyOptions>({ multiple: true, other: 'test' }) // OK
const test7 = await open<MyOptions>({ multiple: true, other: 'test', a: '1' }) // Error

My exposure to TS is too short to judge what the best practices are.

[Fractal-Tess]

Hey there,

So a couple of things to get out of the way.

First of all, this is all my bad for not communicating with the maintainers on the topic as I only ever talked a little bit to Amr, but it was really short we didn't exactly exchange any feedback. However, for the foreseeable future, I will try to open an issue and have an open discussion with the developers before doing random prs.

Credit is where credit is due, and I just have to metion Fabian, who also helped me greatly with this and even found a mistake I had originally left out in the pr, where a non-literal boolean type would give off errors.

All that being said, my stupidity took the better of me and I took @jianxingxuejian 's suggestion on the overload and shipped the changes in a new commit without re-testing the types.

Testing it after the changes, the type for a non-literal boolean value is string | null, which is wrong as it should be string | string[] | null.

I'll revert the last commit as to how it originally was, and I'm still open to new ideas on how to improve the overload, as I myself am not all that experienced with typescript.

Once again, sorry for this!

On a side note, this is what we want:

const s1 = await open() // string | null
const s2 = await open({}) // string | null
const s3 = await open({ multiple: true }) // string[] | null
const s4 = await open({ multiple: false }) // string | null
const s5 = await open({ multiple: Boolean(1) }) // string | string[] | null

[lucasfernog]

Yeah @jianxingxuejian let's go with this approach.