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

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

Merged
merged 3 commits into from Aug 8, 2022

Conversation

arturi
Copy link
Contributor

@arturi arturi commented Aug 4, 2022

No description provided.

@arturi arturi requested a review from Murderlon August 4, 2022 12:12
@arturi arturi changed the base branch from main to 3.x August 4, 2022 12:23
Comment on lines 104 to 113
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

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?

@aduh95 aduh95 changed the base branch from 3.x to main August 4, 2022 16:31
@arturi
Copy link
Contributor Author

arturi commented Aug 5, 2022

I decided to try/catch in uppy.validateRestrictions and return the error there vs changing the Restricter class, because it throws the error in every check, we'd have to change it all for consistency then? What matters is the public method, I think.

@arturi arturi requested review from Murderlon and aduh95 August 5, 2022 22:41
@arturi arturi merged commit 346c2fc into main Aug 8, 2022
@arturi arturi deleted the validate-restrictions-return-error branch August 8, 2022 09:58
This was referenced Aug 16, 2022
github-actions bot added a commit that referenced this pull request Aug 16, 2022
| Package                   |      Version | Package                   |      Version |
| ------------------------- | ------------ | ------------------------- | ------------ |
| @uppy/audio               | 1.0.0-beta.2 | @uppy/progress-bar        | 3.0.0-beta.2 |
| @uppy/aws-s3              | 3.0.0-beta.3 | @uppy/provider-views      | 3.0.0-beta.3 |
| @uppy/aws-s3-multipart    | 3.0.0-beta.4 | @uppy/react               | 3.0.0-beta.4 |
| @uppy/box                 | 2.0.0-beta.2 | @uppy/redux-dev-tools     | 3.0.0-beta.2 |
| @uppy/companion           | 4.0.0-beta.4 | @uppy/remote-sources      | 1.0.0-beta.4 |
| @uppy/companion-client    | 3.0.0-beta.2 | @uppy/screen-capture      | 3.0.0-beta.3 |
| @uppy/compressor          | 1.0.0-beta.3 | @uppy/status-bar          | 3.0.0-beta.3 |
| @uppy/core                | 3.0.0-beta.4 | @uppy/store-default       | 3.0.0-beta.3 |
| @uppy/dashboard           | 3.0.0-beta.4 | @uppy/store-redux         | 3.0.0-beta.3 |
| @uppy/drag-drop           | 3.0.0-beta.2 | @uppy/svelte              | 2.0.0-beta.2 |
| @uppy/drop-target         | 2.0.0-beta.3 | @uppy/thumbnail-generator | 3.0.0-beta.2 |
| @uppy/dropbox             | 3.0.0-beta.2 | @uppy/transloadit         | 3.0.0-beta.5 |
| @uppy/facebook            | 3.0.0-beta.2 | @uppy/tus                 | 3.0.0-beta.3 |
| @uppy/file-input          | 3.0.0-beta.2 | @uppy/unsplash            | 3.0.0-beta.2 |
| @uppy/form                | 3.0.0-beta.2 | @uppy/url                 | 3.0.0-beta.3 |
| @uppy/golden-retriever    | 3.0.0-beta.2 | @uppy/utils               | 5.0.0-beta.1 |
| @uppy/google-drive        | 3.0.0-beta.2 | @uppy/vue                 | 1.0.0-beta.2 |
| @uppy/image-editor        | 2.0.0-beta.3 | @uppy/webcam              | 3.0.0-beta.3 |
| @uppy/informer            | 3.0.0-beta.3 | @uppy/xhr-upload          | 3.0.0-beta.3 |
| @uppy/instagram           | 3.0.0-beta.2 | @uppy/zoom                | 2.0.0-beta.2 |
| @uppy/locales             | 3.0.0-beta.4 | uppy                      | 3.0.0-beta.5 |
| @uppy/onedrive            | 3.0.0-beta.2 |                           |              |

- meta: prepare release workflow for beta versions (Antoine du Hamel)
- @uppy/provider-views: Reset filter input correctly in provider views (Merlijn Vos / #3978)
- @uppy/aws-s3-multipart: Fix when using Companion (Merlijn Vos / #3969)
- @uppy/companion: Companion: bring back default upload protocol (Mikael Finstad / #3967)
- meta: Update CONTRIBUTING.md (Mikael Finstad / #3966)
- meta: fix contributing link (Mikael Finstad / #3968)
- @uppy/companion: enforce usage of uploadUrls (Mikael Finstad / #3965)
- @uppy/utils: Fix webp mimetype (Merlijn Vos / #3961)
- @uppy/locales: Add compressor string translation to Japanese locale (kenken / #3963)
- meta: Fix statement about cropping images in README.md (Mikael Finstad / #3964)
- @uppy/aws-s3-multipart: Fix race condition in `#uploadParts` (Morgan Zolob / #3955)
- @uppy/provider-views: core validateRestrictions: return error directly vs the result/reason obj (Artur Paikin / #3951)
- @uppy/aws-s3: Export AwsS3UploadParameters & AwsS3Options interfaces (Antonina Vertsinskaya / #3956)
- website: convert all website examples to ESM (Antoine du Hamel / #3957)
- @uppy/companion: fix crash if redis disconnects (Mikael Finstad / #3954)
- @uppy/companion: upgrade `ws` version (Antoine du Hamel / #3949)
- @uppy/companion: sort Dropbox response & refactor to async/await (Mikael Finstad / #3897)
- @uppy/utils: modernize `getDroppedFiles` (Antoine du Hamel / #3534)
- @uppy/companion: fix default getKey for non-standalone too (Mikael Finstad / #3945)
- @uppy/aws-s3-multipart: ignore exception inside `abortMultipartUpload` (Antoine du Hamel / #3950)
- @uppy/companion: remove `isobject` from dependencies (Antoine du Hamel / #3948)
- @uppy/compressor: Fix Compressor being broken when no name is in the compressed blob (Artur Paikin / #3947)
- @uppy/core,@uppy/react: Fix all breaking todo comments for 3.0 (Merlijn Vos / #3907)
- @uppy/companion: show deprecation message when using legacy s3 options (Antoine du Hamel / #3944)
- example: fix aws-companion example (Antoine du Hamel / #3850)
HeavenFox pushed a commit to docsend/uppy that referenced this pull request Jun 27, 2023
| Package                   |      Version | Package                   |      Version |
| ------------------------- | ------------ | ------------------------- | ------------ |
| @uppy/audio               | 1.0.0-beta.2 | @uppy/progress-bar        | 3.0.0-beta.2 |
| @uppy/aws-s3              | 3.0.0-beta.3 | @uppy/provider-views      | 3.0.0-beta.3 |
| @uppy/aws-s3-multipart    | 3.0.0-beta.4 | @uppy/react               | 3.0.0-beta.4 |
| @uppy/box                 | 2.0.0-beta.2 | @uppy/redux-dev-tools     | 3.0.0-beta.2 |
| @uppy/companion           | 4.0.0-beta.4 | @uppy/remote-sources      | 1.0.0-beta.4 |
| @uppy/companion-client    | 3.0.0-beta.2 | @uppy/screen-capture      | 3.0.0-beta.3 |
| @uppy/compressor          | 1.0.0-beta.3 | @uppy/status-bar          | 3.0.0-beta.3 |
| @uppy/core                | 3.0.0-beta.4 | @uppy/store-default       | 3.0.0-beta.3 |
| @uppy/dashboard           | 3.0.0-beta.4 | @uppy/store-redux         | 3.0.0-beta.3 |
| @uppy/drag-drop           | 3.0.0-beta.2 | @uppy/svelte              | 2.0.0-beta.2 |
| @uppy/drop-target         | 2.0.0-beta.3 | @uppy/thumbnail-generator | 3.0.0-beta.2 |
| @uppy/dropbox             | 3.0.0-beta.2 | @uppy/transloadit         | 3.0.0-beta.5 |
| @uppy/facebook            | 3.0.0-beta.2 | @uppy/tus                 | 3.0.0-beta.3 |
| @uppy/file-input          | 3.0.0-beta.2 | @uppy/unsplash            | 3.0.0-beta.2 |
| @uppy/form                | 3.0.0-beta.2 | @uppy/url                 | 3.0.0-beta.3 |
| @uppy/golden-retriever    | 3.0.0-beta.2 | @uppy/utils               | 5.0.0-beta.1 |
| @uppy/google-drive        | 3.0.0-beta.2 | @uppy/vue                 | 1.0.0-beta.2 |
| @uppy/image-editor        | 2.0.0-beta.3 | @uppy/webcam              | 3.0.0-beta.3 |
| @uppy/informer            | 3.0.0-beta.3 | @uppy/xhr-upload          | 3.0.0-beta.3 |
| @uppy/instagram           | 3.0.0-beta.2 | @uppy/zoom                | 2.0.0-beta.2 |
| @uppy/locales             | 3.0.0-beta.4 | uppy                      | 3.0.0-beta.5 |
| @uppy/onedrive            | 3.0.0-beta.2 |                           |              |

- meta: prepare release workflow for beta versions (Antoine du Hamel)
- @uppy/provider-views: Reset filter input correctly in provider views (Merlijn Vos / transloadit#3978)
- @uppy/aws-s3-multipart: Fix when using Companion (Merlijn Vos / transloadit#3969)
- @uppy/companion: Companion: bring back default upload protocol (Mikael Finstad / transloadit#3967)
- meta: Update CONTRIBUTING.md (Mikael Finstad / transloadit#3966)
- meta: fix contributing link (Mikael Finstad / transloadit#3968)
- @uppy/companion: enforce usage of uploadUrls (Mikael Finstad / transloadit#3965)
- @uppy/utils: Fix webp mimetype (Merlijn Vos / transloadit#3961)
- @uppy/locales: Add compressor string translation to Japanese locale (kenken / transloadit#3963)
- meta: Fix statement about cropping images in README.md (Mikael Finstad / transloadit#3964)
- @uppy/aws-s3-multipart: Fix race condition in `#uploadParts` (Morgan Zolob / transloadit#3955)
- @uppy/provider-views: core validateRestrictions: return error directly vs the result/reason obj (Artur Paikin / transloadit#3951)
- @uppy/aws-s3: Export AwsS3UploadParameters & AwsS3Options interfaces (Antonina Vertsinskaya / transloadit#3956)
- website: convert all website examples to ESM (Antoine du Hamel / transloadit#3957)
- @uppy/companion: fix crash if redis disconnects (Mikael Finstad / transloadit#3954)
- @uppy/companion: upgrade `ws` version (Antoine du Hamel / transloadit#3949)
- @uppy/companion: sort Dropbox response & refactor to async/await (Mikael Finstad / transloadit#3897)
- @uppy/utils: modernize `getDroppedFiles` (Antoine du Hamel / transloadit#3534)
- @uppy/companion: fix default getKey for non-standalone too (Mikael Finstad / transloadit#3945)
- @uppy/aws-s3-multipart: ignore exception inside `abortMultipartUpload` (Antoine du Hamel / transloadit#3950)
- @uppy/companion: remove `isobject` from dependencies (Antoine du Hamel / transloadit#3948)
- @uppy/compressor: Fix Compressor being broken when no name is in the compressed blob (Artur Paikin / transloadit#3947)
- @uppy/core,@uppy/react: Fix all breaking todo comments for 3.0 (Merlijn Vos / transloadit#3907)
- @uppy/companion: show deprecation message when using legacy s3 options (Antoine du Hamel / transloadit#3944)
- example: fix aws-companion example (Antoine du Hamel / transloadit#3850)
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

Successfully merging this pull request may close these issues.

None yet

3 participants