fix(upload): accept type should be more lenient #1064
Conversation
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. |
src/utility/upload.ts
Outdated
| return accept | ||
| .replace(/\s+/, '') | ||
| .toLowerCase() | ||
| .replace('.jpeg', '.jpg') | ||
| .replace('/jpeg', '/jpg') | ||
| .split(/,/) | ||
| .some(acceptToken => { | ||
| if (acceptToken.startsWith('.')) { | ||
| // tokens starting with a dot represent a file extension | ||
| return file.name | ||
| .toLowerCase() | ||
| .replace(/\.jpeg$/, '.jpg') | ||
| .endsWith(acceptToken) | ||
| } else if (wildcards.includes(acceptToken)) { | ||
| return file.type | ||
| .toLowerCase() | ||
| .startsWith(acceptToken.slice(0, acceptToken.length - 1)) | ||
| } | ||
| return file.type.toLowerCase().replace('/jpeg', '/jpg') === acceptToken | ||
| }) |
There was a problem hiding this comment.
Your replacements aren't global so will only replace the first instance of each.
Regarding JPEG/JPG; JPEG is the only valid MIME type but after playing with Chrome I see browsers are really lenient. If this is the same as other browsers then lenient matching here makes sense and will save devs from dealing with jpg/jpeg typos that don't matter. On the other hand, I would lean towards stricter tests and writing more lenient code (accept=".jpg, .jpeg").
Suggestion with global replacements and lenient matching:
// When matching files, browsers ignore case and consider jpeg/jpg interchangeable.
function normalize(nameOrType: string) {
return nameOrType.toLowerCase().replace(/\bjpg\b/g, 'jpeg')
}
function isAcceptableFile(file: File, accept: string) {
if (!accept) {
return true
}
const wildcards = ['audio/*', 'image/*', 'video/*']
return normalize(accept)
.trim()
.split(/\s*,\s*/)
.some((acceptToken) => {
// tokens starting with a dot represent a file extension
if (acceptToken.startsWith('.')) {
return normalize(file.name).endsWith(acceptToken)
} else if (wildcards.includes(acceptToken)) {
return normalize(file.type).startsWith(acceptToken.replace('*', ''))
}
return normalize(file.type) === acceptToken
})
}There was a problem hiding this comment.
Great suggestions! I incorporated your changes but made a small adjustment to the regex---just to make it a little bit stricter to exclude some cases when jpg appears in the middle of a file or folder name.
| @@ -54,20 +59,28 @@ export async function upload( | |||
| input.removeEventListener('fileDialog', fileDialog) | |||
| } | |||
|
|
|||
| // When matching files, browsers ignore case and consider jpeg/jpg interchangeable. | |||
| function normalize(nameOrType: string) { | |||
| return nameOrType.toLowerCase().replace(/(\.|\/)jpg\b/g, '$1jpeg') | |||
There was a problem hiding this comment.
yes came here to complain about them being case sensitive :D
| new File(['there'], 'there.jpg', {type: 'audio/mp3'}), | ||
| new File(['there'], 'there.csv', {type: 'text/csv'}), | ||
| new File(['there'], 'there.jpg', {type: 'video/mp4'}), | ||
| new File(['hello'], 'image.png', {type: 'image/png'}), |
There was a problem hiding this comment.
put in some image.PNG for good measure
|
@ph-fritsche @Gpx @kentcdodds do you mind taking a look at this? |
|
This looks good to me. I'm not an active maintainer on this package anymore so I don't think I should merge it, but if you don't get a reply in a day or so then I'll go ahead and merge it 👍 |
fae15dd to
44b6498
Compare
|
@ph-fritsche @Gpx @kentcdodds can we merge this? |
|
Sorry nobody has replied on this PR. Unfortunately it looks like in the time since this PR was updated, there were some dependencies that got changed which is likely what is causing the build failure. I'm afraid I don't have more time than to just click the "merge" button on this project, but it won't do any good if the build is failing. So this will have to wait until someone can adjust this PR to fix any build issues. Sorry. |
|
@ktmud I'm sorry about the delay here. Some updates in our dependencies blocked our previous CI. We also learned that the previous CI wasn't sufficient to ensure that bug fixes we apply don't cause new problems in other environments. |
src/utility/upload.ts
Outdated
| if (selectedFiles.length > 0 && files.length === 0) { | ||
| throw new Error('No files were accepted by the `accept` attribute') | ||
| } | ||
|
|
There was a problem hiding this comment.
Throwing an error here is a breaking change.
This reverts commit 7f42994.
44b6498 to
c9d1f7f
Compare
|
🎉 This PR is included in version 14.6.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
What:
Fixes a bug where file upload
input[accept]is not respected when there are spaces in theacceptattribute. Closes #1060.Also raises an error in case the explicitly provided list of files did not have any match.
Why:
HTML attributes are notoriously lenient---if something is valid in HTML, then it should be valid in test cases. The
acceptattribute ignores both letter cases and whitespaces (including new lines). Plus it is very common for users to provide at least 1 space separator for for multi types, e.g.<input type="file" accept="image/png, image/jpeg" />.Raising an error in case of empty filtered file list is more user-friendly than silently fail as a user's test case may fail anyway if an explicit call of
uploadwas not successful.How:
The PR handles these special cases using string replacement and case coercion.
Checklist:
DocumentationN/A as this should be the default expected behavior.