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.

Sign up for GitHub

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

Jump to bottom

Typescript Typed events #3085

Merged
merged 7 commits into from
Aug 17, 2021
Merged

Typescript Typed events #3085

merged 7 commits into from
Aug 17, 2021

Conversation

Hawxy
Copy link
Contributor

@Hawxy Hawxy commented Aug 7, 2021 •
edited
Loading

Closes #2922

Utilizing declaration/interface merging, all events can be typed across all packages. I'm opening this as a draft to collect feedback before I spend time finishing this off.

Positives:

  • Event names are autocompleted within VSCode with no hacks required
  • Merging is additive for any @uppy package the user installs.
  • Easy to add & maintain

Negatives:

  • Unavoidable breaking change for people passing in a metadata structure as part of the generic, as Typescript does not support partial generic inference. I'm unsure if this is an issue, as it was only previously implemented for two events.

Murderlon
Murderlon reviewed Aug 9, 2021
View reviewed changes
Copy link
Member

@Murderlon Murderlon left a comment

Choose a reason for hiding this comment

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

Thanks for contributing! I think this approach makes sense. What do you think, @goto-bus-stop?

@Hawxy Hawxy marked this pull request as ready for review August 14, 2021 12:33
@Hawxy
Copy link
Contributor Author

Hawxy commented Aug 14, 2021

This should be good to go 👍

@Hawxy Hawxy changed the title Proposal: Typescript Typed events Typescript Typed events Aug 14, 2021
Murderlon
Murderlon reviewed Aug 16, 2021
View reviewed changes
Copy link
Member

@Murderlon Murderlon left a comment

Choose a reason for hiding this comment

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

This looks very promising and definitely a stricter approach to event types. I have one question, other than that I think we are good to go.

packages/@uppy/dashboard/types/index.d.ts

export type DashboardFileEditStartCallback<TMeta> = (file: UppyFile<TMeta>) => void;
export type DashboardFileEditCompleteCallback<TMeta> = (file: UppyFile<TMeta>) => void;
declare module '@uppy/core' {
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need to declare this module?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's so the various definitions of the UppyEventMap interface are additive with the initial @uppy/core version, and not treated as if they're from separate modules. Typescript refers to this pattern as module augmentation.

@Murderlon
Copy link
Member

Would you be so kind to describe the breaking type change in a comment in #3100 so we can add it later?

@Hawxy Hawxy mentioned this pull request Aug 16, 2021
Merged
Murderlon
Murderlon reviewed Aug 16, 2021
View reviewed changes
packages/@uppy/core/types/index.d.ts

once<TMeta extends IndexedObject<any> = Record<string, unknown>>(event: 'complete', callback: UploadCompleteCallback<TMeta>): this
on<K extends keyof UppyEventMap, TMeta extends IndexedObject<any>>(event: K, callback: UppyEventMap<TMeta>[K]): this
Copy link
Member

Choose a reason for hiding this comment

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

Will this not be a breaking change if we flip the order? So TMeta and then UppyEventMap?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did consider that, however Typescript does not have partial generic interference (yet), so as soon as you specify one generic the rest must be explicitly specified.

@Murderlon Murderlon merged commit 89f09c8 into transloadit:main Aug 17, 2021
@Murderlon
Copy link
Member

Thank you @Hawxy!

Murderlon added a commit that referenced this pull request Aug 17, 2021
@Murderlon
Merge branch 'main' of https://github.com/transloadit/uppy
980074c 
* 'main' of https://github.com/transloadit/uppy:
  Changelog for 1.31.0 and patches
  Strictly type uppy events (#3085)
  Create `onUnmount` in `UIPlugin` for plugins that require clean up (#3093)
  Companion improve logging (#3103)
  Fix `editFile` locale usage (#3108)
Murderlon added a commit that referenced this pull request Aug 25, 2021
@Murderlon
Merge branch 'main' into 2.0.0-release-post
d74c9d0 
* main: (23 commits)
  Release
  Set node version in `workflows/cdn.yml` to 16.x
  Release
  build: add stylelint (#3124)
  Core: rename allowMultipleUploads to allowMultipleUploadBatches (#3115)
  meta: enforce `no-unused-vars` linter rule (#3118)
  writing-plugins: update example to use `i18nInit` (#3122)
  @uppy/core: reject empty string as valid value for required meta fields (#3119)
  Safely escape <script> injected code in companion `send-token.js` (#3101)
  @uppy/dashboard: fix metafield form validation (#3113)
  Clean up `BACKLOG.md` & add Vimeo as todo
  Add referrer to transloadit.com link (#3116)
  @uppy/locales latest version is 1.22.0 🙈
  Stricter linter (#3095)
  @uppy/aws-s3: refactor to use private fields (#3094)
  build: fix legacy bundle (#3112)
  Fix locales — point to CDN v1.31.0
  Fix typo in `docs/companion.md`
  Changelog for 1.31.0 and patches
  Strictly type uppy events (#3085)
  ...
fstoerkle
fstoerkle reviewed Aug 25, 2021
View reviewed changes
packages/@uppy/core/types/index.d.ts

off(event: Event, callback: (...args: any[]) => void): this
off<K extends keyof UppyEventMap>(event: K, callback: UppyEventMap[K]): this
Copy link

Choose a reason for hiding this comment

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

Can we add a version of off that is similar to on, like the following?

off<K extends keyof UppyEventMap, TMeta extends IndexedObject<any>>(event: K, callback: UppyEventMap<TMeta>[K]): this

Without that, I can‘t pass the same event handler function to on and off:

interface MyMetadata {
  ...
}

const handleUpload = (file: UploadedUppyFile<MyMetadata, unknown>) => { ... };

// this is fine
uppy.on<'upload-success', MyMetadata>('upload-success', handleUpload);

// however, this leads to the TypeScript error below
uppy.off<'upload-success'>('upload-success', handleUpload);

leads to

error TS2345: Argument of type '(file: UploadedUppyFile<Metadata, unknown>) => void' is not assignable to parameter of type 'UploadSuccessCallback<Record<string, unknown>>'.
  Types of parameters 'file' and 'file' are incompatible.
    Type 'UploadedUppyFile<Record<string, unknown>, unknown>' is not assignable to type 'UploadedUppyFile<Metadata, unknown>'.
      Property 'fileId' is missing in type 'Record<string, unknown>' but required in type 'Metadata'.

103       uppy.off<'upload-success'>('upload-success', handleUpload);

If you want, I can also create a ticket for this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for raising this, I'll put up a PR for this shortly.

@Hawxy Hawxy deleted the eventtypes branch August 26, 2021 01:27
@Hawxy Hawxy mentioned this pull request Aug 26, 2021
Merged
@aduh95 aduh95 mentioned this pull request Oct 2, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Murderlon Murderlon Murderlon approved these changes

@fstoerkle fstoerkle fstoerkle left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Stricter typescript definitions
3 participants
@Hawxy @Murderlon @fstoerkle