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

DropzoneDialog: onSave has difficult to use type, fileObjects is now required #184

Closed
zikaeroh opened this issue Jun 4, 2020 · 8 comments

Comments

@zikaeroh
Copy link

zikaeroh commented Jun 4, 2020

Bug Report

Describe the bug

38d5c00 updated the TypeScript types, but the new properties for DropzoneDialog appear to require properties that don't exist, and functions with signatures that aren't (easily) possible to satisfy.

Steps to reproduce

Use DropzoneDialog in a TypeScript project.

My code original code looked like this:

<DropzoneDialog
    acceptedFiles={['.txt']}
    cancelButtonText={'cancel'}
    submitButtonText={'submit'}
    dropzoneClass={classes.dropzone}
    dropzoneText={'Text files, one word per line. Click or drag to upload.'}
    previewGridClasses={{ container: classes.previewGrid }}
    previewText={'Files:'}
    maxFileSize={1000000}
    open={uploadOpen}
    onClose={() => setUploadOpen(false)}
    onSave={async (files: File[]) => {
        setUploadOpen(false);

        // ...
    }}
/>

But the same code now errors with:

Property 'fileObjects' is missing in type '{ acceptedFiles: string[]; cancelButtonText: string; submitButtonText: string; dropzoneClass: string; dropzoneText: string; previewGridClasses: { container: string; }; previewText: string; maxFileSize: number; open: boolean; onClose: () => void; onSave: any; }' but required in type 'DropzoneAreaBaseProps'.ts(2741)

As fileObjects has been added to the props as a non-optional param. I do not believe this prop exists (it appears to be a state variable, not a prop).

Additionally, onSave has an awkward type:

(property) onSave?: (((event: React.SyntheticEvent<Element, Event>) => void) & ((files: File[], event: React.SyntheticEvent<Element, Event>) => void)) | undefined

TS cannot infer the signature of this function, and it seems like a mistake that the first parameter is an event or an array of files. Specifying the type for my function's parameter as File[] gives me the message:

Type '(files: File[]) => Promise<void>' is not assignable to type '((event: SyntheticEvent<Element, Event>) => void) & ((files: File[], event: SyntheticEvent<Element, Event>) => void)'.
  Type '(files: File[]) => Promise<void>' is not assignable to type '(event: SyntheticEvent<Element, Event>) => void'.
    Types of parameters 'files' and 'event' are incompatible.
      Type 'SyntheticEvent<Element, Event>' is missing the following properties from type 'File[]': length, pop, push, concat, and 28 more.ts(2322)

Expected behavior

Between 3.0.0 and 3.1.0, The TS code still compiles.

Versions

  • OS: [e.g. iOS]
  • Browser: [e.g. chrome, safari]
  • @material-ui/core version: [e.g. 4.9.2]
  • material-ui-dropzone version: [e.g. 3.0.1]

Additional context

To reproduce, use clone https://github.com/zikaeroh/codies, and run yarn upgrade && rm -rf node_modules && yarn install in the frontend directory to upgrade. yarn build will print messages (or open the workspace in VS Code and open gameView.tsx).

@zikaeroh zikaeroh changed the title onSave has difficult to use type, fileObjects is now required DropzoneDialog: onSave has difficult to use type, fileObjects is now required Jun 4, 2020
@FilipVavera
Copy link

FilipVavera commented Jun 4, 2020

I am experiencing the same issue.

Versions

  • @material-ui/core version: 4.10.1
  • material-ui-dropzone version: 3.1.0

@wjzhou
Copy link

wjzhou commented Jun 5, 2020

hit this problem too.

It is because fileObjects is required by DropzoneAreaBase, but it is provided internally by DropzoneArea.

However the type annotation of DropzoneAreaBaseProp in index.d.ts inherent that fileObjects from DropzoneAreaBaseProp

@panz3r
Copy link
Contributor

panz3r commented Jun 5, 2020

Hi @zikaeroh and @wjzhou ,

Thanks for you feedback and your support, I just pushed a fix for the TS typings into master.

@zikaeroh
Copy link
Author

zikaeroh commented Jun 5, 2020

Thanks. I just grabbed a version at master, and I think a few properties that had existed in 3.0.0 are now omitted when they shouldn't be, e.g. the onClose property (which is used in the README example, and in my original case).

onSave was the only one that conflicted for DropzoneDialogProps, not onAdd, onDelete, or onClose.

@zikaeroh
Copy link
Author

zikaeroh commented Jun 5, 2020

Type '{ acceptedFiles: string[]; cancelButtonText: string; submitButtonText: string; dropzoneClass: string; dropzoneText: string; previewGridClasses: { container: string; }; previewText: string; maxFileSize: number; open: boolean; onClose: () => void; onSave: (files: File[]) => Promise<...>; }' is not assignable to type 'IntrinsicAttributes & Pick<DropzoneDialogBaseProps, "maxWidth" | "open" | "classes" | "onDrop" | "fullWidth" | "acceptedFiles" | ... 28 more ... | "submitButtonText"> & { ...; } & { ...; }'.
  Property 'onClose' does not exist on type 'IntrinsicAttributes & Pick<DropzoneDialogBaseProps, "maxWidth" | "open" | "classes" | "onDrop" | "fullWidth" | "acceptedFiles" | ... 28 more ... | "submitButtonText"> & { ...; } & { ...; }'.  TS2322

    510 |                             maxFileSize={1000000}
    511 |                             open={uploadOpen}
  > 512 |                             onClose={() => setUploadOpen(false)}
        |                             ^
    513 |                             onSave={async (files) => {
    514 |                                 setUploadOpen(false);
    515 | 

@zikaeroh
Copy link
Author

zikaeroh commented Jun 5, 2020

I think it'd be worth looking at 38d5c00 again to see what the state was before the changes, so the same properties are still exposed.

Also IMO the use of Omit is a bit of a hack; it makes the error messages a bit harder to read as the tooltips will no longer display the entire type, just that long string Pick-ing every item. (Omit I also think is already built into TS, but maybe you're redefining it for older versions of TS without it?) I think there are cleaner ways to do it, but whatever works. 🙂

@panz3r
Copy link
Contributor

panz3r commented Jun 5, 2020

Hi @zikaeroh ,

Thanks for the quick feedback 👍 , I just updated the typings.

Omit I also think is already built into TS, but maybe you're redefining it for older versions of TS without it?

Exactly for that reason and unfortunately keeping the TS typings updated is a bit of pain (hoping to fix this with a future migration to TS 😬 ).

@zikaeroh
Copy link
Author

zikaeroh commented Jun 5, 2020

Thanks, my project now builds again!

Exactly for that reason and unfortunately keeping the TS typings updated is a bit of pain (hoping to fix this with a future migration to TS 😬 ).

Yes, being able to write the whole thing and tests in TS will certainly help prevent API drift.

anthonyraymond pushed a commit to anthonyraymond/material-ui-dropzone that referenced this issue Jun 11, 2020
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

No branches or pull requests

4 participants