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

simplify findDOMElements #4957

Merged
merged 4 commits into from
Feb 27, 2024
Merged

simplify findDOMElements #4957

merged 4 commits into from
Feb 27, 2024

Conversation

mifi
Copy link
Contributor

@mifi mifi commented Feb 24, 2024

why do we need generics here?

Copy link
Contributor

github-actions bot commented Feb 24, 2024

Diff output files
diff --git a/packages/@uppy/utils/lib/findDOMElement.js b/packages/@uppy/utils/lib/findDOMElement.js
index 848cf8d..014ff83 100644
--- a/packages/@uppy/utils/lib/findDOMElement.js
+++ b/packages/@uppy/utils/lib/findDOMElement.js
@@ -1,5 +1,5 @@
 import isDOMElement from "./isDOMElement.js";
-export default function findDOMElement(element, context) {
+function findDOMElement(element, context) {
   if (context === void 0) {
     context = document;
   }
@@ -11,3 +11,4 @@ export default function findDOMElement(element, context) {
   }
   return null;
 }
+export default findDOMElement;

Copy link
Contributor

@aduh95 aduh95 left a comment

Choose a reason for hiding this comment

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

This makes the types less precise, what's the upside of this?

@mifi
Copy link
Contributor Author

mifi commented Feb 26, 2024

not sure why this precision is useful. it makes the function harder to understand and adds the need to @ts-expect-error three times. do you have an example of when this generic would be useful? i cannot find any in its current usages

@mifi
Copy link
Contributor Author

mifi commented Feb 27, 2024

ok, now I understand, thanks. still learning typescript & generics!

@mifi mifi closed this Feb 27, 2024
@mifi mifi reopened this Feb 27, 2024
improved typings
Copy link
Contributor

@aduh95 aduh95 left a comment

Choose a reason for hiding this comment

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

Interesting that TS would need overload for that, but as long as it works 👍

Two small nits to make the types even more precise:

packages/@uppy/utils/src/findDOMElement.ts Outdated Show resolved Hide resolved
packages/@uppy/utils/src/findDOMElement.ts Outdated Show resolved Hide resolved
Co-authored-by: Antoine du Hamel <antoine@transloadit.com>
@mifi
Copy link
Contributor Author

mifi commented Feb 27, 2024

i found it here: https://stackoverflow.com/a/52818072/6519037
I think using generics with conditional types is just a different way of expressing multiple overloads instead of having to write them all out

@aduh95 aduh95 merged commit 12fb3ae into main Feb 27, 2024
16 checks passed
@aduh95 aduh95 deleted the simplify-finddomelements branch February 27, 2024 17:08
@github-actions github-actions bot mentioned this pull request Feb 28, 2024
github-actions bot added a commit that referenced this pull request Feb 28, 2024
| Package                | Version | Package                | Version |
| ---------------------- | ------- | ---------------------- | ------- |
| @uppy/box              |   2.2.1 | @uppy/onedrive         |   3.2.1 |
| @uppy/companion-client |   3.7.4 | @uppy/progress-bar     |   3.1.0 |
| @uppy/core             |   3.9.3 | @uppy/provider-views   |  3.10.0 |
| @uppy/dashboard        |   3.7.5 | @uppy/status-bar       |   3.3.0 |
| @uppy/file-input       |   3.1.0 | @uppy/utils            |   5.7.4 |
| @uppy/form             |   3.2.0 | @uppy/xhr-upload       |   3.6.4 |
| @uppy/image-editor     |   2.4.4 | uppy                   |  3.23.0 |
| @uppy/informer         |   3.1.0 |                        |         |

- @uppy/form: migrate to TS (Merlijn Vos / #4937)
- @uppy/box: fetchPreAuthToken in box too (Mikael Finstad / #4969)
- @uppy/progress-bar: refactor to TypeScript (Mikael Finstad / #4921)
- @uppy/onedrive: fix custom oauth2 credentials for onedrive (Mikael Finstad / #4968)
- @uppy/companion-client,@uppy/utils,@uppy/xhr-upload: improvements for #4922 (Mikael Finstad / #4960)
- @uppy/utils: fix various type issues (Mikael Finstad / #4958)
- @uppy/provider-views: migrate to TS (Merlijn Vos / #4919)
- @uppy/utils: simplify `findDOMElements` (Mikael Finstad / #4957)
- @uppy/xhr-upload: fix getResponseData regression (Merlijn Vos / #4964)
- @uppy/informer: migrate to TS (Merlijn Vos / #4967)
- @uppy/core: remove unused import (Antoine du Hamel / #4972)
- @uppy/image-editor: remove default target (Merlijn Vos / #4966)
- @uppy/angular: Build fixes (Mikael Finstad / #4959)
- meta: Fix flaky e2e test (Murderlon)
- meta: fix e2e flake (Mikael Finstad / #4961)
- meta: add support for `Fragment` short syntax (Antoine du Hamel / #4953)
- @uppy/file-input: refactor to TypeScript (Antoine du Hamel / #4954)
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.

2 participants