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.

Sign up for GitHub

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

Jump to bottom

core validateRestrictions: return error directly vs the result/reason obj #3951

Merged
merged 3 commits into from Aug 8, 2022
Merged

core validateRestrictions: return error directly vs the result/reason obj #3951

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1d7676c
core validateRestrictions: return error directly vs the result/reason…
arturi Aug 4, 2022
5d69fc7
Refacrtor to actually use restrictionError instead of {reason, message}
arturi Aug 4, 2022
750a7da
Return error instead of throwing
arturi Aug 5, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
9 changes: 1 addition & 8 deletions packages/@uppy/core/src/Uppy.js
View file
Open in desktop
Expand Up @@ -390,14 +390,7 @@ class Uppy {
}

validateRestrictions (file, files = this.getFiles()) {
// TODO: directly return the Restriction error in next major version.
// we create RestrictionError's just to discard immediately, which doesn't make sense.
try {
this.#restricter.validate(file, files)
return { result: true }
} catch (err) {
return { result: false, reason: err.message }
}
return this.#restricter.validate(file, files)
}

#checkRequiredMetaFieldsOnFile (file) {
Expand Down
17 changes: 4 additions & 13 deletions packages/@uppy/core/src/Uppy.test.js
View file
Open in desktop
Expand Up @@ -1795,20 +1795,11 @@ describe('src/Core', () => {
size: 270733,
}

const validateRestrictions1 = core.validateRestrictions(newFile)
const validateRestrictions2 = core2.validateRestrictions(newFile)

expect(validateRestrictions1).toMatchObject(
{
result: false,
reason: 'This file is smaller than the allowed size of 293 KB',
},
expect(() => core.validateRestrictions(newFile)).toThrow(
'This file is smaller than the allowed size of 293 KB',
)
expect(validateRestrictions2).toMatchObject(
{
result: false,
reason: 'You can only upload: image/png',
},
expect(() => core2.validateRestrictions(newFile)).toThrow(
'You can only upload: image/png',
)
})

Expand Down
14 changes: 10 additions & 4 deletions packages/@uppy/provider-views/src/Browser.jsx
View file
Open in desktop
Expand Up @@ -101,10 +101,16 @@ function Browser (props) {
})}

{files.map((file) => {
const validated = validateRestrictions(
remoteFileObjToLocal(file),
[...uppyFiles, ...currentSelection],
)
let validated
try {
validateRestrictions(
remoteFileObjToLocal(file),
[...uppyFiles, ...currentSelection],
)
validated = { result: true }
} catch (err) {
validated = { result: false, reason: err.message }
}
Copy link
Member

Choose a reason for hiding this comment

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

You are just moving the strange API down 😅

The idea is that we don't need a boolean, we only have an error variable that is undefined or defined. Then change everything that consumes this to check whether err is defined instead of result:true.

Two options:

  1. Don't throw but return error in restricter.validate. Then we have undefined or RestrictionError as a value to deal with
                const err = validateRestrictions(
                    remoteFileObjToLocal(file),
                    [...uppyFiles, ...currentSelection],
                  )
  1. Keep restricter.validate like it is, but then you need to try/catch wrap it always with a let

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but what does it matter if anyone can use the actual error from Core? The public API is a real error like you wanted. I can get rid of the result thing and just check for the error, yeah.

Copy link
Member

Choose a reason for hiding this comment

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

We should change our code too, yes. The question is which approach of the two mentioned above we should take?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated to use the restrictionError directly.

Copy link
Member

@Murderlon Murderlon Aug 4, 2022 •
edited

Choose a reason for hiding this comment

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

I'm still considering that the first option may be better. If the function is not a promise and the expected value is an error, not an unexpected error, than perhaps we don't need to 'blow up' the program with throw. We want the value after all. What do you think @aduh95 @arturi?

Copy link
Member

Choose a reason for hiding this comment

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

We can change the method name so the behavior is clearer (e.g. getRestrictionError or something)

Copy link
Member

Choose a reason for hiding this comment

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

I wouldn't base it on what I'd call imaginary performance issues, as in, such things will never be the bottleneck in any web app. I also prefer validateRestrictions still. The type will become clear in JSDoc and/or TS anyway. But let's go for option 1 then :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So more or less same, but return instead of throw, so no need for weird try/catch. Ok.

Copy link
Member

Choose a reason for hiding this comment

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

Might be nice to make things consistent and take along validateMinNumberOfFiles too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

validateMinNumberOfFiles is internal, only used in Core so far, and I didn't feel like changing just one check in Restricter, for consistency we’d have to change all of them to return?


return Item({
id: file.id,
Expand Down