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

Provider views rewrite (.files, .folders => .partialTree) #5050

Merged
merged 187 commits into from
Jun 21, 2024

Conversation

lakesare
Copy link
Contributor

@lakesare lakesare commented Mar 29, 2024

Description


TODOs for next PRs


Notes to reviewers

  • I made deliberate effort not to touch the folder structure at all (for ease of reviewing & because we didn't set our minds on which one we'd prefer yet).
  • Don't worry about 1k of added lines - this PR actually reduces the number of lines by a few hundred lines - the increase is due to test files/comments.
  • We discussed a shared .env in Slack - it's underway, but not yet fully ready.
    If you need access to a particular provider - message me, and I'll send you the credentials.

This PR spans 2.5k lines across 46 files.

I separated some PRs from this PR, however further separation will make it harder to review rather than easier, this is largely a holistic change.

I think a sane way to review this PR (apart from looking through "Files changed" on github) would be to look through the code in /uppy/packages/@uppy/provider-views folder locally in your editor - just read the files and see if they make sense/if something worries you. Most of these files were updated/rewritten.

GoogleDrive
- travelling down into folders works
- checking a file works
- breadcrumbs DONT work
Copy link

socket-security bot commented Jun 13, 2024

👍 Dependency issues cleared. Learn more about Socket for GitHub ↗︎

This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored.

View full report↗︎

@Murderlon Murderlon mentioned this pull request Jun 18, 2024
Copy link
Contributor

@mifi mifi left a comment

Choose a reason for hiding this comment

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

many great improvements in code quality and ux improvements in the file selector! seems to be working nicely. the code changes are huge, but sometimes big refactors are needed to move forward. as mentioned by others it's a bit hard to review in without first getting a deep understanding of the code, which will probably take a few hours/days (I don't have time for that now). but I trust you have tested this well and it seems to have good test coverage so it should be good to merge so people can start trying it out.

export type PartialTreeStatusFile = 'checked' | 'unchecked'
export type PartialTreeStatus = PartialTreeStatusFile | 'partial'

export type PartialTreeFile = {
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe id, parentId and data can be pulled out into a common type, e.g. PartialTreeBase and then PartialTreeFile and PartialTreeFolderNode can extend from it? to reduce duplication and make it more clear which fields they have in common

Copy link
Contributor Author

@lakesare lakesare Jun 19, 2024

Choose a reason for hiding this comment

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

I think having PartialTreeBase would add a level of indirection without making these types more readable/easier to work with.

In general - I avoided having many types here, because I want people who work on these files to largely remember existing types by heart (that's just 4 types - PartialTree, ~File, ~FolderNode, ~FolderRoot).

E.g., one really seductive type was PartialTreeDisplayedNode = ~File | ~FolderNode (because these are the only nodes that can be seen in the interface, meaning most functions expect ~File | ~FolderNode as an argument), but I decided against it to cut down on the number of types the developer would have to memorize.

@lakesare
Copy link
Contributor Author

lakesare commented Jun 20, 2024

Tested with

  • GoogleDrive
  • GooglePhotos
  • Box
  • Dropbox
  • Facebook
  • Instagram
  • OneDrive
  • Unsplash
  • Zoom

The testing process consists of the following procedure:
1. sign in 2. check deeply nested files 3. download them 4. sign out.

@lakesare lakesare mentioned this pull request Jun 20, 2024
11 tasks
Copy link

New dependencies detected. Learn more about Socket for GitHub ↗︎

Package New capabilities Transitives Size Publisher
npm/@uppy-dev/[email protected] None 0 0 B

View full report↗︎

@lakesare lakesare merged commit 4dc2860 into 4.x Jun 21, 2024
20 of 21 checks passed
@lakesare lakesare deleted the lakesare/provider-folders branch June 21, 2024 00:44
github-actions bot added a commit that referenced this pull request Jun 27, 2024
| Package                |       Version | Package                |       Version |
| ---------------------- | ------------- | ---------------------- | ------------- |
| @uppy/audio            |  2.0.0-beta.7 | @uppy/image-editor     |  3.0.0-beta.6 |
| @uppy/aws-s3           |  4.0.0-beta.8 | @uppy/instagram        |  4.0.0-beta.7 |
| @uppy/box              |  3.0.0-beta.8 | @uppy/onedrive         |  4.0.0-beta.8 |
| @uppy/companion        | 5.0.0-beta.11 | @uppy/provider-views   | 4.0.0-beta.10 |
| @uppy/companion-client |  4.0.0-beta.8 | @uppy/react            |  4.0.0-beta.8 |
| @uppy/core             | 4.0.0-beta.11 | @uppy/screen-capture   |  4.0.0-beta.6 |
| @uppy/dashboard        | 4.0.0-beta.11 | @uppy/transloadit      | 4.0.0-beta.10 |
| @uppy/drop-target      |  3.0.0-beta.6 | @uppy/unsplash         |  4.0.0-beta.8 |
| @uppy/dropbox          |  4.0.0-beta.9 | @uppy/url              |  4.0.0-beta.8 |
| @uppy/facebook         |  4.0.0-beta.7 | @uppy/utils            |  6.0.0-beta.9 |
| @uppy/file-input       |  4.0.0-beta.6 | @uppy/vue              |  2.0.0-beta.4 |
| @uppy/form             |  4.0.0-beta.5 | @uppy/webcam           |  4.0.0-beta.9 |
| @uppy/golden-retriever |  4.0.0-beta.6 | @uppy/xhr-upload       |  4.0.0-beta.7 |
| @uppy/google-drive     |  4.0.0-beta.1 | @uppy/zoom             |  3.0.0-beta.7 |
| @uppy/google-photos    |  0.2.0-beta.2 | uppy                   | 4.0.0-beta.13 |

- @uppy/companion: implement facebook app secret proof (Mikael Finstad / #5249)
- @uppy/provider-views: `Loader.tsx` - delete the file (Evgenia Karunus / #5284)
- @uppy/vue: fix passing of `props` (Antoine du Hamel / #5281)
- @uppy/google-photos: fix various issues (Mikael Finstad / #5275)
- @uppy/transloadit: fix strict type errors (Antoine du Hamel / #5271)
- @uppy/transloadit: simplify plugin to always run a single assembly (Merlijn Vos / #5158)
- meta: update Yarn version and npm deps (Antoine du Hamel / #5269)
- docs: prettier: 3.2.5 -> 3.3.2 (Antoine du Hamel / #5270)
- @uppy/provider-views: Provider views rewrite (.files, .folders => .partialTree) (Evgenia Karunus / #5050)
- @uppy/react: TS strict mode (Merlijn Vos / #5258)
- meta: simplify `build:ts` script (Antoine du Hamel / #5262)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants