Custom validators #321

Closed
okonet opened this Issue Jan 11, 2017 · 17 comments

Projects

None yet

5 participants

@okonet
Owner
okonet commented Jan 11, 2017

react-dropzone provides some simple validation out of the box like accept, minSize and maxSize but it lacks rejection reason (#257 and #319) so it's impossible to react to this rejection without the duplicating the validations in the user's component's code.

I have been thinking about implementing some kind of custom validator functions that you could then compose. Each such validator function should have a generic API. Something like this:

import { validate, assertMaxFileSize, assertMinFileSize, asserFileType } from 'react-dropzone'

function onValidate(files) {
  const { acceptedFiles, rejectedFiles } = validate(assertMaxFileSize, assertMinFileSize, asserFileType)(files)
  return [ acceptedFiles, rejectedFiles ]
}

const MyDropZone = (props) => {
  return <Dropzone onValidate={onValidate} />
}

This would allow create custom validators and react to rejections appropriately in the UI.

Thoughts?

@kandros
kandros commented Jan 11, 2017 edited

I don't know, I believe simple validation like file-size and file-type is better as it is now, handled by the library and based on props, for more complex validation you can already handle all of them in the onDrop function, where you have access to the files array already.

The example is not actually exposing the failure reasons.

what showed in #257 adding the failure reasons to the file reference, and export a reason lookup-table (to match rejected files with a filter function) looks more intuitive

@okonet
Owner
okonet commented Jan 11, 2017

The example is simplified of course, and the use case with the rejection reason should be handled by it as well (I open for suggestions here). I'm afraid that simple validations are often not enough and I want to keep the surface API of the library minimal.

I realize the current approach is a simpler one, but coupling the lib with validation purposes tends to increase the number of props and different combinations of them which IMO leads to complexity.

For instance, https://github.com/enyo/dropzone is very popular but it tries to solve all use cases which leads to enormous amount of options: http://www.dropzonejs.com/#configuration-options

@kandros
kandros commented Jan 11, 2017 edited

oh I see now, I tried to implement something and totally came up with the same solution, basically onValidate will pass [ acceptedFiles, rejectedFiles ] to the current implementation of onDrop right?

@okonet
Owner
okonet commented Jan 11, 2017

Yeah, this is what I was thinking about. What is your experience with the similar implementation?

@kandros
kandros commented Jan 11, 2017

Looks good to me, passing [ acceptedFiles, rejectedFiles ] instead of a list of validation functions makes it easier to handle errors or a per file basis instead of relying on the library

what about array of errors to each rejectedFile? so that we can handle multiple errors on a single file and push our internally
we could validate internally minSize,maxSize and accept, if props are present, would really like to avoid doing this manually

@okonet
Owner
okonet commented Jan 11, 2017

Can you write down an example of how it would look like?

Regarding the internal validation: yes, some validations could be included for compatibility reasons. The question is how the API should look like for simple extensions.

Let's say, I'm implementing a dropzone that should only accept:

  1. PMG images
  2. Smaller than 2 MB
  3. Longest side is smaller than 500px

It seems like the onValidate is again too much bootstrap code and probably validation function should be file based?

Something like:

function myValidator(file: File): Boolean {
  const longerSide = file.dimensions  // calculate longer side pseudo-code
  if (longerSise > 500) {
    return false
  }
  return true
}

const MyDropZone = (props) => {
  return <Dropzone customValidator={myValidator} accept="image/png" />
}

But this will lack the reason information again :( I don't know a better way of providing rejection information without mutating the input as with the Promise but this again might be too much:

function myValidator(file: File, props: Props) {
  // Implement a function that returns a Promise?
  return new Promise((resolve, reject) => {
    if (longerSise > 500) {
      reject("Reason for rejection")
    }
    // Do whaterver you want with the file Object here
    resolve(file)
  })
}

const MyDropZone = (props) => {
  return <Dropzone customValidator={myValidator} accept="image/png" />
}

Thoughts?

@okonet
Owner
okonet commented Jan 11, 2017

Another frustration point is how accept works. Since it's built-in HTML5 feature we can display rejection on file mismatch ahead of file being dropped. This won't work with any validations (fileSize doesn't for instance) until the file is actually dropped.

I don't know how to communicate this in a way it's more clear.

@MichaelLeeHobbs
MichaelLeeHobbs commented Jan 11, 2017 edited

I've been contemplating this for close to an hour. The question is where do you stop? Do you keep library simple and have it do one thing well and let the users handle calls to validation among other things or do you start expanding. I could think of a long list of useful features: validation, transformer(aka zip or image processing), uploader(think slingshot), progress bar, etc. Here is an example of how we are using it in an MVP project: https://github.com/MichaelLeeHobbs/dropzoneMVP

I'd keep it simple return (err, result) let the library users handle the rest. If the errObj is not null/undefined then call the onError function otherwise call the onDrop function.

{error}, {acceptedFiles: [[file, msg]], rejectedFiles: [[file, msg]]}

If error is undefined/null you pass the result obj to the onDrop, otherwise pass both to onError.

If you want to go deeper. onValidate > onTransform > onDrop > onUpload(onStart, onAbort, onDone, onProgress, onError). Then rename the project to Ultimate Drop Zone :-)

@okonet
Owner
okonet commented Jan 11, 2017

@kandros I think your confirmation use case could be solved with the Promise as well. What do you think?

@okonet
Owner
okonet commented Jan 11, 2017

@MichaelLeeHobbs my goal here is to not expand but even shrink the API.

So, just to make it clear, I 'm thinking about some generic way of adding a custom file-based validation before the onDrop method gets called. This validation step would be used internally as well to perform file type check etc. but I'd like it to me extendable so I could remove minSize and maxSize as props.

So basically I want to integrate what you do here https://github.com/MichaelLeeHobbs/dropzoneMVP/blob/master/dropZone/DropZone.jsx#L79-L88 into the library since it already has some validation step and calls onDrop with acceptedFiles and rejectedFiles. This should allow users to hook into the Dropzone's validation step and be able to alter those acceptedFiles and rejectedFiles based on their custom rules.

Regarding the syntax, I'm not sure if callback or Promise is a better API here. Promises can be used to do some remote-type validation but they increase the complexity since now the onDrop is called in async way. Thoughts?

@mistadikay

My opinion: react-dropzone (and components in general) should do one thing and do it good. So, I propose to leave validation to the outside world and keep only native stuff like 'accept'. I use react-dropzone and I validate everything separately (both sync and async validation) and it works just fine.

@kandros
kandros commented Jan 12, 2017

Comments made me rethink about the validations, even simpler one should be done by the user, all we need is a function before onDrop that passes files to onDrop, without distinguishing accepted and rejected anymore

@okonet
Owner
okonet commented Jan 12, 2017

Yeah I even feel that there is no need for a separate function since this can and should be done in the onDrop callback. The question is if dropzone should still handle accept attribute and call the callback with 2 arrays instead of just one?

I think it should since accept is the part of HTML5 and dropzone wraps <input> but stuff like size validation should go away in favor of custom validation.

@mistadikay

I think it should since accept is the part of HTML5 and dropzone wraps but stuff like size validation should go away in favor of custom validation.

Yes, I totally agree. 👍

@MichaelLeeHobbs

@okonet As others have suggested leave it to custom validation and maybe update the doc's with some example of how to do validation.

@okonet
Owner
okonet commented Jan 12, 2017

Yes, thanks everyone for the feedback! I'll do that in the next major release since this will be a breaking change.

@rrhvella

I think it makes sense to leave the validation to the user.

@okonet okonet closed this Jan 20, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment