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

@uppy/core: add generic to getPlugin #5247

Merged
merged 2 commits into from
Jun 13, 2024
Merged

@uppy/core: add generic to getPlugin #5247

merged 2 commits into from
Jun 13, 2024

Conversation

Murderlon
Copy link
Member

@Murderlon Murderlon commented Jun 12, 2024

It's a common use case to getPlugin and access something on it. Currently it's a bit awkward:

const state = (uppy.getPlugin('Transloadit') as Transloadit<any,any>).getPluginState()

This PR reintroduces the generic on getPlugin, so it's an improvement but also backwards compatibility with 3.x:

const state = uppy.getPlugin<Transloadit<any,any>>('Transloadit').getPluginState()

Unfortunately you must pass the generics, which should be optional but I don't know how to achieve that.

@Murderlon Murderlon requested review from mifi and aduh95 June 12, 2024 10:37
@Murderlon Murderlon self-assigned this Jun 12, 2024
Copy link
Contributor

Diff output files
No diff

@mifi
Copy link
Contributor

mifi commented Jun 12, 2024

wouldn't it be more elegant if we return the correct type based on the string value passed to getPlugin?

from chatgpt:

In TypeScript, you can use conditional types to return a different type based on a constant string value passed as an argument to a function. Here's an example:

type TypeMap = {
  type1: number;
  type2: string;
  type3: boolean;
};

function getValue<T extends keyof TypeMap>(type: T): TypeMap[T] {
  let value: TypeMap[T];

  if (type === 'type1') {
    value = 123 as TypeMap[T];
  } else if (type === 'type2') {
    value = 'hello' as TypeMap[T];
  } else if (type === 'type3') {
    value = true as TypeMap[T];
  }

  return value;
}

let a = getValue('type1'); // a is of type number
let b = getValue('type2'); // b is of type string
let c = getValue('type3'); // c is of type boolean

In this example, TypeMap is a mapping of string keys to types. The getValue function takes a key of TypeMap as an argument and returns the corresponding type from TypeMap. The actual implementation of getValue would depend on your specific use case.

@aduh95
Copy link
Contributor

aduh95 commented Jun 12, 2024

wouldn't it be more elegant if we return the correct type based on the string value passed to getPlugin?

I don't think that's possible, the id is user-provided, we can't know in advance what type it's going to be – or if there's going to be any plugin with that ID.

@aduh95
Copy link
Contributor

aduh95 commented Jun 12, 2024

We could also have a static method e.g. Transloadit.getPlugin(uppy, 'Transloadit'); another idea would be to change the function signature to be uppy.getPlugin(Transloadit, 'Transloadit'), and we could make the second argument optional, defaulting to arg1.constructor.name EDIT: I just tried that locally, and failed.

@Murderlon
Copy link
Member Author

wouldn't it be more elegant if we return the correct type based on the string value passed to getPlugin?

Indeed not possible afaik. There might be one small chance at getting something to work like that, which is this todo I wrote here:

// TODO: can we use namespaces in other plugins to populate this?
// eslint-disable-next-line @typescript-eslint/no-empty-interface
interface Plugins extends Record<string, Record<string, unknown> | undefined> {}

Something along these lines, in every plugin:

declare module '@uppy/core' {
  export interface Plugins<M extends Meta, B extends Body> {
    'Transloadit': Transloadit<M, B>
  }
}

@mifi
Copy link
Contributor

mifi commented Jun 12, 2024

I don't think that's possible, the id is user-provided, we can't know in advance what type it's going to be – or if there's going to be any plugin with that ID.

but won't users 99% of the time write getPlugin with a constant string literal, like getPlugin('transloadit') (not getPlugin(someVariable))? and typescript is perfectly able to statically analyse string literals and create a proper return type for our built in plugins. of course it won't work with third party plugins but at least it will make the api more developer friendly 99% of the time

@mifi
Copy link
Contributor

mifi commented Jun 12, 2024

We could also have a static method e.g. Transloadit.getPlugin(uppy, 'Transloadit');

that could work

alternatively we could implement a new static API for accessing our built in plugins, like

uppy.transloadit?: TransloaditPlugin
uppy.dropbox?: DropboxPlugin
// and so on

or

uppy.plugins.transloadit?: TransloaditPlugin
uppy.plugins.dropbox?: DropboxPlugin
// and so on

@aduh95
Copy link
Contributor

aduh95 commented Jun 12, 2024

@mifi what I meant is that users can provide a different id, and we can't really predict what those are going to be (heck they could use the string 'Transloadit' to refer to the Dashboard)

this.id = this.opts.id || 'AwsS3Multipart'

@mifi
Copy link
Contributor

mifi commented Jun 12, 2024

@mifi what I meant is that users can provide a different id, and we can't really predict what those are going to be (heck they could use the string 'Transloadit' to refer to the Dashboard)

maybe we should prevent people from using the string transloadit to refer to Dashboard etc (throw an error in such case), because in that case how can one even call getPlugin to get the real Transloadit plugin if the id transloadit refers to Dashboard?

then we can predict the return type in most cases, but of course if they have a custom plugin our return types can be unknown or some generic plugin type

@aduh95
Copy link
Contributor

aduh95 commented Jun 12, 2024

in that case how can one even call getPlugin to get the real Transloadit plugin if the id transloadit refers to Dashboard?

The Transloadit plugin would also have its own id, and that's what they'd need to pass

Let's not overthink it, this PR is already an improvement, no user has complained about this.

@Murderlon
Copy link
Member Author

@mifi it's likely impossible. Maybe we could hack together something in the way I described, but then it won't work with different id's as @aduh95 pointed out, which wouldn't make it an acceptable solution.

@Murderlon Murderlon merged commit b6a0250 into 4.x Jun 13, 2024
19 checks passed
@Murderlon Murderlon deleted the getPlugin-type branch June 13, 2024 08:17
Murderlon added a commit that referenced this pull request Jun 17, 2024
* 4.x:
  Renames & `eslint-disable react/require-default-props` removal (#5251)
  coalesce options `bucket` and `getKey` (#5169)
  @uppy/aws-s3: add `endpoint` option (#5173)
  @uppy/locales: fix `fa_IR` export (#5241)
  improve companion logging (#5250)
  Release: uppy@4.0.0-beta.11 (#5243)
  @uppy/core: add generic to `getPlugin` (#5247)
  docs: add 4.x migration guide (#5206)
  @uppy/transloadit: also fix outdated assembly transloadit:result (#5246)
  docs - fix typo in the url
  @uppy/core: set default for Body generic (#5244)
  Release: uppy@3.26.1 (#5242)
  docs: clarify assemblyOptions for @uppy/transloadit (#5226)
  meta: Improve aws-node example readme (#4753)
  @uppy/react: remove `react:` prefix from `id` & allow `id` as a prop (#5228)
  Added translation string (it_IT) (#5237)
  docs: correct allowedMetaFields (#5227)
  @uppy/transloadit: fix transloadit:result event (#5231)
  docs: remove `extraData` note from migration guide (#5219)
  @uppy/provider-views: fix wrong font for files (#5234)
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.

3 participants