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

feat: allow dropping a folder into the dropzone #1066

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

gvdp
Copy link

@gvdp gvdp commented Jan 31, 2024

Recreated #834 after updating master with all the latest changes.

As for the question about how in-demand this is: we've needed it for one of our apps and it's been in production for over a year now. I made it so there is an opt-in boolean which needs to be enabled, so by default nothing should change for exisiting users.

Seems also like this is one of the oldest issues on this repo where some people have asked for it: #2

Copy link
Collaborator

@gilest gilest left a comment

Choose a reason for hiding this comment

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

Hey @gvdp. Thank you for reopening this – appreciate your contributions as always ❤️

so by default nothing should change for exisiting users.

That's true.. But adding a feature will increase the bundle size for all addon users. When we add a feature we are committing as maintainers to support it for a long time, too

For testing purposes I have pushed this feature branch to docs/folder-drop which deploys a public docsite here:
https://docs-folder-drop.ember-file-upload.pages.dev/demo

I'll keep that updated so that we can test across different browsers and devices.

I did some light testing in Safari, Firefox, Chrome and Edge on Mac OS and Windows and found a few issues:

  • Dropping a directory of directories throws and leaves the dropzone in a broken state (dropzone.active stays true afterwards)
    • Interested in your suggestion of how we might handle this
      • recursively open subdirectories and add their files, too
      • only add files one-level deep but do not throw an error when nested folders are present
  • The directory itself is being reported as an UploadFile in callbacks
    • this might be useful programatically, it's a significant API change and something that's not modelled as part of this addon. For example there's no way to "upload" the directory from the queue
    • maybe we can get notified about directory details at the dropzone-event level, but I'm not sure we want those items being added to the queue
  • In some browsers (Firefox on Mac OS) .DS_Store files are being added. Not sure there's ever a reason this would be desired. I guess we should consider how to handle dot files in general, too

In terms of the implementation, it would be great to reduce the usage of the TypeScript as keyword. That would give us more confidence about the robustness of this feature

Once we've resolved these issues, we'll also need to add documentation of this feature

  • API reference -> FileDropzone
  • Documentation -> Testing
  • (New page) Documentation -> Add directories via Drag and Drop

ember-file-upload/src/components/file-dropzone.ts Outdated Show resolved Hide resolved
ember-file-upload/src/components/file-dropzone.ts Outdated Show resolved Hide resolved
ember-file-upload/src/interfaces.ts Outdated Show resolved Hide resolved
Comment on lines 99 to 104
export async function dragAndDropDirectory(
selector: string,
folderName: string,
filesInDirectory: (File | Blob)[],
...singleFiles: (File | Blob)[]
) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder if we should use an options object here. From the call site the two types of files (in directory and single) are not very clear.

Copy link
Author

@gvdp gvdp Feb 5, 2024

Choose a reason for hiding this comment

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

Fair call, it's not very clear indeed. Would you make 1 opts object? Or keep the selector and/or folderName and make an object for the rest? I also noticed there is no example / test really using the ...singleFiles argument.

I'm doubting if the folderName even needs to be here, but that'll probably be relevant for the bigger remark about the directory itself being reported as UploadFile

ember-file-upload/src/system/data-transfer-wrapper.ts Outdated Show resolved Hide resolved
@gvdp
Copy link
Author

gvdp commented Feb 5, 2024

Hi @gilest , thanks for the comments, appreciate the effort. I fixed some of the smaller code comments already. Not sure how you usually commit those? I made a few small extra commits now but they can be easily squashed.

  • The directory of directories is indeed something which does not work. In our own application this is not necessary and we give the end user an explicit notice of this. I don't think including everything recursively is the way to go, maybe we should just throw or show a specific, relevant error to the user?
  • The directory itself being reported as UploadFile is indeed not ideal, I seem to remember we did this for a reason but it's already some time ago and I don't know what it was :-) . As a File object it's not possible to see the difference between a file and a directory, but there is the isDirectory prop which we do already use. Maybe we should just not include the directoryFile in the array being sent here? I think this would not break our current implementation. That would probably also mean the directoryName prop in the test helper is no longer relevant.
  • The .DS_store files are also annoying. In our implementation we catch and filter those out in the consuming application. Not sure we should just ignore all dotfiles, sometimes it can be intentional that these are uploaded. Maybe we should just include a warning in the documentation that it is the responsibility of the consuming application to handle all hidden files?

I'll of course also add documentation and API reference stuff once we've resolved these open issues. Thanks again for the review!

Copy link
Collaborator

@gilest gilest left a comment

Choose a reason for hiding this comment

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

As always, thanks for your continued work on this.

Sorry this review took so long – it's hard to find a chunk of time to dedicate to a complex feature like this.

For testing purposes I have pushed this feature branch to docs/folder-drop which deploys a public docsite here:
https://docs-folder-drop.ember-file-upload.pages.dev/demo

Have made some general suggestions, especially around improving resilience when a file fails to read.

Please don't be discouraged by my feedback 😅 This is library code so it's better that we consider failure modes and use-cases now than make breaking changes later

Will respond to remaining design questions in a separate comment.

interface FutureProofDataTransferItem extends DataTransferItem {
getAsEntry?: () => FileSystemDirectoryEntry;
}
Copy link
Collaborator

@gilest gilest Feb 23, 2024

Choose a reason for hiding this comment

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

According to the spec this may fail and return null. We probably want to have our types reflect that.

The method aborts and returns null if the dropped item isn't a file, or if the DataTransferItem object is not in read or read/write mode.

Suggested change
interface FutureProofDataTransferItem extends DataTransferItem {
getAsEntry?: () => FileSystemDirectoryEntry;
}
interface FutureProofDataTransferItem extends DataTransferItem {
getAsEntry?: () => FileSystemDirectoryEntry | null;
}

Copy link
Author

Choose a reason for hiding this comment

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

Added the null case

Comment on lines 70 to 74
if (typeof item.getAsEntry === 'function') {
return item.getAsEntry();
}

return item.webkitGetAsEntry() as FileSystemDirectoryEntry;
Copy link
Collaborator

@gilest gilest Feb 23, 2024

Choose a reason for hiding this comment

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

Shorter way of doing this... Maybe the type signature of this function may needs to union null also

Suggested change
if (typeof item.getAsEntry === 'function') {
return item.getAsEntry();
}
return item.webkitGetAsEntry() as FileSystemDirectoryEntry;
return item.getAsEntry?.() ?? item.webkitGetAsEntry();

Copy link
Author

Choose a reason for hiding this comment

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

Updated

Comment on lines 79 to 81
getEntry(item)
?.createReader()
?.readEntries(async (entries: FileSystemEntry[]) => {
Copy link
Collaborator

@gilest gilest Feb 23, 2024

Choose a reason for hiding this comment

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

We probably want to allow for the result of getEntry to return null. In which case we should probably reject this promise.

Copy link
Author

Choose a reason for hiding this comment

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

Added a Promise rejection when getEntry is falsy. Not sure it should maybe have a clearer message.

).catch((err) => {
throw err;
});
resolve(readFiles.filter((file) => file !== undefined) as File[]);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Safer to filter out missing elements with a truthiness check

Suggested change
resolve(readFiles.filter((file) => file !== undefined) as File[]);
resolve(readFiles.filter(Boolean) as File[]);

Copy link
Author

Choose a reason for hiding this comment

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

Didn't know you could use the Boolean constructor like this. Updated.

Comment on lines 116 to 122
files.push(...flattenedFileArray);
} else {
const droppedFiles: File[] = Array.from(this.dataTransfer?.files ?? []);
files.push(...droppedFiles);
}

return files;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure we need to define a files variable and mutate it. Each of these branches could just be an outright return

Copy link
Author

Choose a reason for hiding this comment

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

Made it immediate returns

@@ -43,6 +47,81 @@ export default class DataTransferWrapper {
return this.files.length ? this.files : this.items;
}

async getFilesAndDirectories() {
Copy link
Collaborator

@gilest gilest Feb 23, 2024

Choose a reason for hiding this comment

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

[Perf] All of the child functions defined within getFilesAndDirectories do not rely on class state or local variable closure.

Because of this, they only need to be defined once in memory, rather than every time this methodn is called.

Let's move all of the child functions to module scope (you can put them in the top of this file).

I think extracting them also helps with readability of this feature.

Copy link
Author

Choose a reason for hiding this comment

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

Moved all the functions to the top.

async getFilesAndDirectories() {
const files: File[] = [];

const readEntry = async (entry: FileSystemEntry): Promise<File> => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This function returns a promise and does not use the await keyword.

Suggested change
const readEntry = async (entry: FileSystemEntry): Promise<File> => {
const readEntry = (entry: FileSystemEntry): Promise<File> => {

Copy link
Author

Choose a reason for hiding this comment

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

Good catch. Removed the async keyword.
There is an eslint rule which can alert these cases, maybe you can add this to you configuration:
https://eslint.org/docs/latest/rules/require-await

}

export async function dragAndDropDirectory(
selector: string,
Copy link
Collaborator

@gilest gilest Feb 23, 2024

Choose a reason for hiding this comment

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

Let's follow the same convention as the other test helpers and allow this to be a selector or an element.

Suggested change
selector: string,
target: string | HTMLElement,

Copy link
Author

Choose a reason for hiding this comment

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

Updated

@gilest
Copy link
Collaborator

gilest commented Feb 23, 2024

Separate comment with "design feedback"

I fixed some of the smaller code comments already. Not sure how you usually commit those? I made a few small extra commits now but they can be easily squashed.

Extra commits is no problem. The whole feature will be squash-merged anyway 🙂

  • The directory of directories is indeed something which does not work. In our own application this is not necessary and we give the end user an explicit notice of this. I don't think including everything recursively is the way to go, maybe we should just throw or show a specific, relevant error to the user?

My suggestion: let's add the simplest implementation for now, which we can extend later (ideally without too much breaking change).

Given that, I think it would be ok include only files from the top-level of the dropped directory, and ignore subdirectories.

This is more flexible than any nested directories causing an error. For example, this avoids cause users to have to re-arrange their local directories just to upload a group of files which would be very frustrating.

  • The directory itself being reported as UploadFile is indeed not ideal, I seem to remember we did this for a reason but it's already some time ago and I don't know what it was :-) . As a File object it's not possible to see the difference between a file and a directory, but there is the isDirectory prop which we do already use. Maybe we should just not include the directoryFile in the array being sent here? I think this would not break our current implementation. That would probably also mean the directoryName prop in the test helper is no longer relevant.

Given the "simplest implementation" suggestion I would agree with this and prefer that we not report directories for now. If needed, we can model this later as an enhancement (along with recursion).

  • The .DS_store files are also annoying. In our implementation we catch and filter those out in the consuming application. Not sure we should just ignore all dotfiles, sometimes it can be intentional that these are uploaded. Maybe we should just include a warning in the documentation that it is the responsibility of the consuming application to handle all hidden files?

Agree with your suggestion – worth documenting. We shouldn't make this decision for users as we can't anticipate their use-cases.

@gvdp
Copy link
Author

gvdp commented Feb 29, 2024

As always, thanks for your continued work on this.

Sorry this review took so long – it's hard to find a chunk of time to dedicate to a complex feature like this.

For testing purposes I have pushed this feature branch to docs/folder-drop which deploys a public docsite here: https://docs-folder-drop.ember-file-upload.pages.dev/demo

Have made some general suggestions, especially around improving resilience when a file fails to read.

Please don't be discouraged by my feedback 😅 This is library code so it's better that we consider failure modes and use-cases now than make breaking changes later

Will respond to remaining design questions in a separate comment.

No problem at all, I appreciate the feedback and I agree it's better to be thorough now. You're the one who probably has to maintain all this later :-).

@gvdp
Copy link
Author

gvdp commented Feb 29, 2024

Separate comment with "design feedback"

I fixed some of the smaller code comments already. Not sure how you usually commit those? I made a few small extra commits now but they can be easily squashed.

Extra commits is no problem. The whole feature will be squash-merged anyway 🙂

  • The directory of directories is indeed something which does not work. In our own application this is not necessary and we give the end user an explicit notice of this. I don't think including everything recursively is the way to go, maybe we should just throw or show a specific, relevant error to the user?

My suggestion: let's add the simplest implementation for now, which we can extend later (ideally without too much breaking change).

Given that, I think it would be ok include only files from the top-level of the dropped directory, and ignore subdirectories.

This is more flexible than any nested directories causing an error. For example, this avoids cause users to have to re-arrange their local directories just to upload a group of files which would be very frustrating.

  • The directory itself being reported as UploadFile is indeed not ideal, I seem to remember we did this for a reason but it's already some time ago and I don't know what it was :-) . As a File object it's not possible to see the difference between a file and a directory, but there is the isDirectory prop which we do already use. Maybe we should just not include the directoryFile in the array being sent here? I think this would not break our current implementation. That would probably also mean the directoryName prop in the test helper is no longer relevant.

Given the "simplest implementation" suggestion I would agree with this and prefer that we not report directories for now. If needed, we can model this later as an enhancement (along with recursion).

  • The .DS_store files are also annoying. In our implementation we catch and filter those out in the consuming application. Not sure we should just ignore all dotfiles, sometimes it can be intentional that these are uploaded. Maybe we should just include a warning in the documentation that it is the responsibility of the consuming application to handle all hidden files?

Agree with your suggestion – worth documenting. We shouldn't make this decision for users as we can't anticipate their use-cases.

I removed the Promise rejection when there is a subdirectory so the user doesn't get an unnecessary error. I am wondering if we should log a warning or something in this case or just silently ignore this case? Or will just documenting this behavior be enough? I already added a small code comment as well.

Also removed the reporting of the dropped directory itself, now only the files included will be reported. I think I remember now that we used this to keep track of the name of the dropped folder, but that's not super important and we can solve on our end.
Everything still seems to work for me on the demo page :-)

Unrelated comment: I noticed while working on this some of the scripts in the root package.json don't quite work. When you run pnpm run test there is a prepare script being executed which doesn't exist, and when running pnpm test:watch:test-app the --server option also doesn't exist.

@gilest
Copy link
Collaborator

gilest commented Mar 19, 2024

Hi @gvdp . Thank you for your comment and contribution. I had completely missed this notification in a sea of others. Apologies. I will try to review soon!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants