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

Add Methods type #1066

Merged
merged 1 commit into from
Jun 5, 2024
Merged

Add Methods type #1066

merged 1 commit into from
Jun 5, 2024

Conversation

ehmicky
Copy link
Collaborator

@ehmicky ehmicky commented May 17, 2024

Fixes #1062.

This PR adds the Methods['execa'], Methods['execaNode'] and Methods['$'] types. Those are the types of execa/execa(options), and so on.

This is useful for TypeScript users passing bound/curried instances of execa around.

Using ReturnType<execa> does not work due to the overloading of the execa() function (execa(options), execa(file, args, options), template string syntax). So, ReturnType<execa> is the return type of execa(options) but also of execa(file, args, options) (i.e. a ResultPromise), which makes assigning it not work.

import type {ExecaScriptCommon, ExecaScriptSync} from './script.js';

// The types seem to fail unless type aliases are used.
// We do not know why.
Copy link
Collaborator Author

@ehmicky ehmicky May 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I cannot explain this.
The types fail unless type aliases are used. 🤔

Even if those types were directly exported, without using the Methods object type.

@Shakeskeyboarde
Copy link

Shakeskeyboarde commented May 17, 2024

On mobile right now, so I'll check tomorrow, but I'm a little skeptical that the new exports are subscript indexes of the Methods type, instead of just the types themselves. Also, the new exports aren't generic, so while the return type from currying might be assignable to them, I think we're loosing type information.

@ehmicky
Copy link
Collaborator Author

ehmicky commented May 17, 2024

Also, the new exports aren't generic, so while the return type from currying might be assignable to them, I think we're loosing type information.

The generic type is the Options being used. For example, when the encoding option is 'buffer', result.stdout is a Uint8Array.

Those method types should not be used where type inference is possible instead. When using type inference, the Options generic type is inferred and kept. For example:

import {execa as execa_} from 'execa';

// `execa` type is inferred as `Execa<{preferLocal: true}>`
const execa = execa_({preferLocal: true});

So the main reason for those new types is when the Execa methods are passed as arguments to a function. If we added the Options generic type, users could specify them to avoid losing type information, as you mention.

import {execa as execa_, type Methods} from 'execa';

const runCommand = async (execaMethod: Methods['execa']<{preferLocal: true}>) => {
  // ...
}

const execa = execa_({preferLocal: true});
await runCommand(execa);

However, using typeof would be simpler and result in more accurate types.

import {execa as execa_, type Methods} from 'execa';

const runCommand = async (execaMethod: typeof execa) => {
  // ...
}

const execa = execa_({preferLocal: true});
await runCommand(execa);

So the only use case IMHO would be for functions that enforce that the caller uses a specific option. For instance, the example above would enforce that preferLocal is true.

That being said, we are using Options as a generic type in most of the other types (ResultPromise, Subprocess, Result, ExecaError), so that does make sense to be consistent. I will update the PR accordingly and add the Options generic type.

On mobile right now, so I'll check tomorrow, but I'm a little skeptical that the new exports are subscript indexes of the Methods type, instead of just the types themselves.

This is mostly a stylistic issue. The exported types would be the same, so this should not change your usage of those types.

The reasoning behind grouping those types together under a Methods type is:

  • Making it clear what those types using the word "method".
  • Re-using the same name as the exported method. In particular, $'s type is named $, as opposed to be renamed something like ExecaScript.
  • Grouping those types since they have the same purpose.
  • Avoid two exported variables differing only by case, e.g. execa value vs Execa type, as this could create some confusion.

However, I am happy to update this PR to calling those ExecaMethod, ExecaNodeMethod and ExecaScriptMethod instead.


cc @sindresorhus

@ehmicky ehmicky marked this pull request as draft May 17, 2024 16:37
@ehmicky
Copy link
Collaborator Author

ehmicky commented May 17, 2024

Adding the Options generic type seems to run into some type issue. This seems a little complicated, and might be some time to fix, so I am putting this PR back to draft.

@ehmicky ehmicky force-pushed the type-methods branch 7 times, most recently from 5d8ce73 to af4a534 Compare June 4, 2024 20:47
@ehmicky ehmicky force-pushed the type-methods branch 2 times, most recently from 55c326e to 2aca70a Compare June 4, 2024 23:39
@ehmicky ehmicky marked this pull request as ready for review June 4, 2024 23:43
@ehmicky
Copy link
Collaborator Author

ehmicky commented Jun 4, 2024

I found out two TypeScript bugs which were blocking this PR, but worked around it.

The new exported types are: ExecaMethod, ExecaNodeMethod, ExecaScriptMethod, ExecaSyncMethod, ExecaScriptSyncMethod. They take an optional Options/SyncOptions as a generic type.

Ready for review @sindresorhus 👍

@sindresorhus sindresorhus merged commit a2e4dbe into main Jun 5, 2024
14 checks passed
@sindresorhus sindresorhus deleted the type-methods branch June 5, 2024 10:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Export Execa and ExecaScript types.
3 participants