fix: do not allow to upload files with invalid formats

[LauraBeatris]

What: Don't allow to upload files with invalid formats.

Why: Files with formats that aren't accepted aren't uploaded in an input file, in fact, it's not even allowed to select them, so the tests should also follow the same behavior.

How: Verify if the element has an accept attribute, if it does, verify if the file/files match the MIME types specified.

Checklist:

• Documentation
• Tests
• Typings
• Ready to be merged
  

[codecov]

Codecov Report

Merging #558 (1a2192e) into master (ad427a6) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master      #558   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           13        13           
  Lines          699       710   +11     
  Branches       221       223    +2     
=========================================
+ Hits           699       710   +11     

Impacted Files	Coverage Δ
src/upload.js	100.00% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ad427a6...c7b60b9. Read the comment docs.

[LauraBeatris]

Still need to add a test case for the following MIME types: image/*,video/* , or in general, the ones that accept all types of extensions

[LauraBeatris]

Still need to add a test case for the following MIME types: image/*,video/* , or in general, the ones that accept all types of extensions

Handled at a7c715a

I've decided to use regex to validate cases where multiple images, videos, and audio formats are supplied, like "flags"

Given a accept attribute like the following: image/* and file type: image/jpg

  const fileMimeType = type.match(/\w+\/+/g) // image
  const isValidMimeType = acceptAttribute.includes(fileMimeType) // true - image/* includes image 

[LauraBeatris]

Still need to improve the validation logic for MIME types - After thinking a little bit more, it would be better to use another dependency to validate the file type. It would return code and make it easier to maintain.

[ph-fritsche]

First of all: Thanks for this pull request :)

As the user is not able to select files that don't match the accept field, I'm not sure if any changes are necessary - if it is just up to the author of the test to be sensible about it.
userEvent methods are "simulate this action by the user" and we don't need to simulate actions that a user can not perform because the browser already prevents them from doing so.
But maybe it makes the life of the test author easier when reusing some fixtures and the necessary changes are small.
@nickmccurdy What do you think?

The changes in upload are too spread out over the function.
The change should be just one line at line 16, applying the filter before the slice.

I don't think validating the types here is complex enough to command outsourcing it in another dependency.
Correct me if I'm wrong, but the only allowed variants with wildcard are:

• type/* which can be easily translated to new RegExp(`^${type}\\/`)
• * (which is equivalent to not providing an accept attribute)

[LauraBeatris]

@ph-fritsche

userEvent methods are "simulate this action by the user" and we don't need to simulate actions that a user can not perform because the browser already prevents them from doing so.

I really agree with that but, there are some situations worth discussing. For instance, if a button is disabled, the user won't be able to click on it, so the browser is already preventing the action - but upload also checks for the disable attribute.

Another situation is, the user may have 4 files and just one of them is not allowed to be selected, so the action will be performed anyway, in that way, I think that it's better if upload follows the same behavior.

Let's also say that an engineer removes the accept attribute of an input component, or removes an specific file type - The tests would break.

[LauraBeatris]

I don't think validating the types here is complex enough to command outsourcing it in another dependency.
Correct me if I'm wrong, but the only allowed variants with wildcard are:

type/* which can be easily translated to new RegExp(^${type}\\/)

• (which is equivalent to not providing an accept attribute)

I think that * is not a valid value 🤔 , MDN docs just mention audio/*, image/* and video/*

[ph-fritsche]

Let's follow the (living standard) spec: https://html.spec.whatwg.org/multipage/input.html#file-upload-state-(type=file)

• anything starting with a dot . - true if the File.name ends on it
• audio/*, image/*, video/* - true if the File.type starts with it (excluding the *)
• anything else - true if the File.type is equal

[jeysal]

Could throwing or console.erroring (not sure if there's precedent for that in testing lib) be a way to get that benefit Laura mentioned (tests breaking when removing an accept attribute) while also not causing potentially hard to understand behavior where the tester may not know why some files are just being lost?

[ph-fritsche]

@LauraBeatris I opened a PR with suggested changes here: LauraBeatris#1

[ph-fritsche]

Could throwing or console.erroring (not sure if there's precedent for that in testing lib) be a way to get that benefit Laura mentioned (tests breaking when removing an accept attribute) while also not causing potentially hard to understand behavior where the tester may not know why some files are just being lost?

Removing an accept attribute would not break the test. Adding one would, but would also be the expected behavior of the test if you exclude a file per accept that you tried to upload.

[jeysal]

Removing an accept attribute would not break the test. Adding one would, but would also be the expected behavior of the test if you exclude a file per accept that you tried to upload.

Sorry, I worded that incorrectly - removing a type from the list of accepted types.
And yes, you would want the test to break if that type is being uploaded in it, that's the point.

Wdyt about throw/console.error? Might save some people from getting stuck trying to figure out why their uploads are silently ignored

[ph-fritsche]

And yes, you would want the test to break if that type is being uploaded in it, that's the point.

The test would break if it tests for the uploaded files to be applied in that input/change event.

Wdyt about throw/console.error? Might save some people from getting stuck trying to figure out why their uploads are silently ignored

This would also mean that the user could not use userEvent.upload to test the accept attribute.
So there would be exactly the same situation as before this PR.

edit: Maybe put a bit too strict: Yes he could test it, but a change would meaning an expect().toThrow would be needed on test. Which might mask exceptions in the tested code. So no, an exception or console.error is no solution.

[LauraBeatris]

I'll fix the coverage soon