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/utils: refactor to TS #4699

Merged
merged 134 commits into from
Nov 6, 2023
Merged

@uppy/utils: refactor to TS #4699

merged 134 commits into from
Nov 6, 2023

Conversation

aduh95
Copy link
Member

@aduh95 aduh95 commented Sep 25, 2023

@@ -1,28 +1,30 @@
const DATA_URL_PATTERN = /^data:([^/]+\/[^,;]+(?:[^,]*?))(;base64)?,([\s\S]*)$/

export default function dataURItoBlob (dataURI, opts, toFile) {
export default function dataURItoBlob (dataURI: string, opts: { mimeType: string, name: string }, toFile: boolean) {
Copy link
Contributor

@nickrttn nickrttn Sep 27, 2023

Choose a reason for hiding this comment

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

For a library I would consider it a best practice to use explicit return types, not inferred return types. There's a typescript-eslint rule to enforce that. WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

not sure if dataURItoBlob is a public API though? we don't document it as part of the uppy api

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't see why it would change anything that we're a library, tsc will still produce the appropriate return type when we ask it to emit the .d.ts file. Or is it to avoid having unintentional semver-minor change slip through?

Copy link
Contributor

@nickrttn nickrttn Sep 27, 2023

Choose a reason for hiding this comment

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

I don't think you should make the distinction between public vs. private in this API but just make a choice between inferred and explicit and stick with it throughout the codebase. Explicit return types throughout will lead to better generated .d.ts files in the end.

Copy link
Member

Choose a reason for hiding this comment

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

my two cents: simple functions without conditional return types are fine to infer. Otherwise type it. If that's too inconsistent or forgetful we could add the eslint rule yes.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we just don’t force anything using rules, just allow people to do what they prefer?

Copy link
Contributor

Choose a reason for hiding this comment

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

Explicit return types throughout will lead to better generated .d.ts files in the end.

Shouldn't we expect TS to generate better return type than us humans? 🤔

Why would an inferred type be better than you describing exactly what you want/need a function to return?

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we just don’t force anything using rules, just allow people to do what they prefer?

That's going to lead to messy and inconsistent type definitions.

Copy link
Member Author

Choose a reason for hiding this comment

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

Explicit return types throughout will lead to better generated .d.ts files in the end.

Shouldn't we expect TS to generate better return type than us humans? 🤔

Why would an inferred type be better than you describing exactly what you want/need a function to return?

Because it would describe what the function actually returns instead of what I think it does

Copy link
Contributor

Choose a reason for hiding this comment

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

Which might not be what you want it to return. If you specify the return type you'll get TS errors if you do not return exactly that.

Base automatically changed from ts-packages-support to main October 17, 2023 13:38
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.

packages/@uppy/utils/src/UppyFile.ts Show resolved Hide resolved
"emitDeclarationOnly": false,
"noEmit": true
},
"include": ["./package.json", "./types/*.ts"],
Copy link
Member

Choose a reason for hiding this comment

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

For utils now that we are refactoring everything it once, yes. But this comment started about the include and I still think that's incorrect.

Murderlon and others added 9 commits November 1, 2023 16:02
* main:
  meta: fix js2ts check
* main:
  More image editor improvements (#4676)
  @uppy/react: add useUppyState (#4711)
  Release: uppy@3.18.1 (#4760)
  Bump jsonwebtoken from 8.5.1 to 9.0.0 in /packages/@uppy/companion (#4751)
  Bump react-devtools-core from 4.25.0 to 4.28.4 (#4756)
  Bump webpack from 5.74.0 to 5.88.2 (#4740)
  Bump @babel/traverse from 7.22.5 to 7.23.2 (#4739)
  @uppy/core: fix `sideEffects` declaration (#4759)
  Release: uppy@3.18.0 (#4754)
  @uppy/aws-s3-multipart: fix `TypeError` (#4748)
  Bump tough-cookie from 4.1.2 to 4.1.3 (#4750)
  example: simplify code by using built-in `throwIfAborted` (#4749)
  @uppy/aws-s3-multipart: pass `signal` as separate arg for backward compat (#4746)
  meta: fix TS integration (#4741)
* main:
  Companion+client stability fixes, error handling and retry (#4734)
  add getBucket metadata argument (#4770)
  @uppy/core: simplify types with class generic (#4761)
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.

I don't know why this is still failing:

packages/@uppy/core/types/index.d.ts:1:28 - error TS2307: Cannot find module '@uppy/utils' or its corresponding type declarations.

1 import * as UppyUtils from '@uppy/utils'
                             ~~~~~~~~~~~~~

packages/@uppy/utils/src/Translator.ts Show resolved Hide resolved
packages/@uppy/utils/src/Translator.ts Show resolved Hide resolved
packages/@uppy/utils/src/FileProgress.ts Show resolved Hide resolved
packages/@uppy/utils/src/Translator.ts Outdated Show resolved Hide resolved
Comment on lines +57 to +59
gz: 'application/gzip' as const,
dmg: 'application/x-apple-diskimage' as const,
}
Copy link
Member

@Murderlon Murderlon Nov 3, 2023

Choose a reason for hiding this comment

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

I think we can get rid of all these as const's with this

Suggested change
gz: 'application/gzip' as const,
dmg: 'application/x-apple-diskimage' as const,
}
gz: 'application/gzip' as const,
dmg: 'application/x-apple-diskimage' as const,
} as Readonly<Record<string, string>>

Copy link
Member Author

Choose a reason for hiding this comment

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

That doesn't do the same thing at all, as const is not there to indicate the value is read-only

Copy link
Member

Choose a reason for hiding this comment

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

why is it there then?

Copy link
Member Author

Choose a reason for hiding this comment

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

Here's the first search result for as const typescript: https://stackoverflow.com/a/66993654

If you leave it off, the compiler will use its default type inference behavior, which will possibly result in a wider or more general type.

Copy link
Contributor

Choose a reason for hiding this comment

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

Here's a video from Matt Pocock describing as const:

https://www.youtube.com/watch?v=6M9aZzm-kEc&pp=ygUUbWF0dCBwb2NvY2sgYXMgY29uc3Q%3D

@aduh95 aduh95 merged commit 51ecc66 into main Nov 6, 2023
18 checks passed
@aduh95 aduh95 deleted the utils-to-ts branch November 6, 2023 14:01
@github-actions github-actions bot mentioned this pull request Nov 8, 2023
github-actions bot added a commit that referenced this pull request Nov 8, 2023
| Package                | Version | Package                | Version |
| ---------------------- | ------- | ---------------------- | ------- |
| @uppy/aws-s3           |   3.5.0 | @uppy/provider-views   |   3.7.0 |
| @uppy/aws-s3-multipart |   3.9.0 | @uppy/react            |   3.2.0 |
| @uppy/companion        |  4.11.0 | @uppy/transloadit      |   3.4.0 |
| @uppy/companion-client |   3.6.0 | @uppy/tus              |   3.4.0 |
| @uppy/core             |   3.7.0 | @uppy/url              |   3.4.0 |
| @uppy/dashboard        |   3.7.0 | @uppy/utils            |   5.6.0 |
| @uppy/image-editor     |   2.3.0 | @uppy/xhr-upload       |   3.5.0 |
| @uppy/locales          |   3.4.0 | uppy                   |  3.19.0 |

- @uppy/dashboard: Remove uppy-Dashboard-isFixed when uppy.close() is invoked (Artur Paikin / #4775)
- @uppy/core,@uppy/dashboard: don't cancel all files when clicking "done" (Mikael Finstad / #4771)
- @uppy/utils: refactor to TS (Antoine du Hamel / #4699)
- @uppy/locales: locales: add ca_ES (ordago / #4772)
- @uppy/companion: Companion+client stability fixes, error handling and retry (Mikael Finstad / #4734)
- @uppy/companion: add getBucket metadata argument (Mikael Finstad / #4770)
- @uppy/core: simplify types with class generic (JokcyLou / #4761)
- @uppy/image-editor: More image editor improvements (Evgenia Karunus / #4676)
- @uppy/react: add useUppyState (Merlijn Vos / #4711)
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.

None yet

6 participants