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

@uppy/transloadit: improve fetch error handling #3637

Merged
merged 2 commits into from
Apr 19, 2022

Conversation

aduh95
Copy link
Contributor

@aduh95 aduh95 commented Apr 11, 2022

Returning a non-2xx code is not a NetworkError, it's a rejection from the server and we should handle it as such.

Returning a non-2xx code is not a `NetworkError`, it's a rejection from
the server and we should handle it as such.
@aduh95 aduh95 added the safe to test Add this label on trustworthy PRs to spawn the e2e test suite label Apr 11, 2022
@aduh95 aduh95 requested a review from goto-bus-stop April 11, 2022 09:32
@github-actions github-actions bot removed the safe to test Add this label on trustworthy PRs to spawn the e2e test suite label Apr 11, 2022
Comment on lines +16 to +24
if (!assembly.error) throw serverError

const error = new Error(assembly.error)
error.details = assembly.message
error.assembly = assembly
if (assembly.assembly_id) {
error.details += ` Assembly ID: ${assembly.assembly_id}`
}
throw error
Copy link
Contributor

Choose a reason for hiding this comment

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

this stuff is quite specific to assembly creation I think 🤔 it probably works fine like this but if it could be done only for createAssembly() calls it would be even better

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I couldn't find a "good" way to special case the assembly creation request (I don't want to create a function just for it, I don't want to leak the response object to the error object, I don't think it's worth using WeakMaps), so I end up using url.endsWith('/assemblies') which may have some false positive. Open to better suggestions :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, I was thinking of putting the response object on the error object, but if you dont want to do that I'm OK with this solution 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I’ve read somewhere that it was bad practice as it could create memory leaks to have Response objects on the error.

@aduh95 aduh95 added the safe to test Add this label on trustworthy PRs to spawn the e2e test suite label Apr 11, 2022
@github-actions github-actions bot removed the safe to test Add this label on trustworthy PRs to spawn the e2e test suite label Apr 11, 2022
@aduh95 aduh95 added the safe to test Add this label on trustworthy PRs to spawn the e2e test suite label Apr 19, 2022
@github-actions github-actions bot removed the safe to test Add this label on trustworthy PRs to spawn the e2e test suite label Apr 19, 2022
@aduh95 aduh95 added the safe to test Add this label on trustworthy PRs to spawn the e2e test suite label Apr 19, 2022
@github-actions github-actions bot removed pending end-to-end tests safe to test Add this label on trustworthy PRs to spawn the e2e test suite labels Apr 19, 2022
@aduh95 aduh95 merged commit dceac85 into transloadit:main Apr 19, 2022
@aduh95 aduh95 deleted the transloadit-fetch-error-handling branch April 19, 2022 17:15
@github-actions github-actions bot mentioned this pull request Apr 27, 2022
github-actions bot added a commit that referenced this pull request Apr 27, 2022
| Package                | Version | Package                | Version |
| ---------------------- | ------- | ---------------------- | ------- |
| @uppy/aws-s3-multipart |   2.2.2 | @uppy/file-input       |   2.0.6 |
| @uppy/box              |   1.0.6 | @uppy/form             |   2.0.5 |
| @uppy/companion        |   3.5.1 | @uppy/locales          |   2.0.9 |
| @uppy/compressor       |   0.2.5 | @uppy/transloadit      |   2.1.5 |
| @uppy/core             |   2.1.9 | @uppy/utils            |   4.0.7 |
| @uppy/drag-drop        |   2.0.7 | @uppy/vue              |   0.4.7 |
| @uppy/drop-target      |   1.1.3 | @uppy/robodog          |   2.5.4 |
| @uppy/dropbox          |   2.0.6 | uppy                   |   2.9.4 |
| @uppy/facebook         |   2.0.6 |                        |         |

- @uppy/locales: Plural translation in cs_CZ local (JakubHaladej / #3666)
- @uppy/vue: Add license field to package.json in @uppy/vue (Tobias Trumm / #3664)
- meta: Add todo comments (Murderlon)
- @uppy/facebook: refactor to ESM (Antoine du Hamel / #3653)
- meta: locale-pack: refactor to use more parallel processing (Antoine du Hamel / #3630)
- @uppy/file-input: refactor to ESM (Antoine du Hamel / #3652)
- meta: sign requests sent to Transloadit in e2e suite (Antoine du Hamel / #3656)
- meta: add `VITE_TRANSLOADIT_SECRET` for e2e (Antoine du Hamel)
- meta: Update BACKLOG.md (Artur Paikin)
- @uppy/form: refactor to ESM (Antoine du Hamel / #3654)
- @uppy/dropbox: refactor to ESM (Antoine du Hamel / #3651)
- meta: sign requests sent to Transloadit in dev env (Antoine du Hamel / #3517)
- @uppy/drop-target: refactor to ESM (Antoine du Hamel / #3648)
- @uppy/core: fix `TypeError` when file was removed (Antoine du Hamel / #3650)
- @uppy/drag-drop: refactor to ESM (Antoine du Hamel / #3647)
- meta: update outdated files (Antoine du Hamel / #3646)
- @uppy/compressor: Set meta on file compression (Camilo Forero / #3644)
- @uppy/transloadit: improve fetch error handling (Antoine du Hamel / #3637)
- @uppy/box: refactor to ESM (Antoine du Hamel / #3643)
- @uppy/utils: Fix getFileType for dicom images (Merlijn Vos / #3610)
- @uppy/aws-s3-multipart: Add `companionCookiesRule` type to @uppy/aws-s3-multipart (Mauricio Ribeiro / #3623)
HeavenFox pushed a commit to docsend/uppy that referenced this pull request Jun 27, 2023
| Package                | Version | Package                | Version |
| ---------------------- | ------- | ---------------------- | ------- |
| @uppy/aws-s3-multipart |   2.2.2 | @uppy/file-input       |   2.0.6 |
| @uppy/box              |   1.0.6 | @uppy/form             |   2.0.5 |
| @uppy/companion        |   3.5.1 | @uppy/locales          |   2.0.9 |
| @uppy/compressor       |   0.2.5 | @uppy/transloadit      |   2.1.5 |
| @uppy/core             |   2.1.9 | @uppy/utils            |   4.0.7 |
| @uppy/drag-drop        |   2.0.7 | @uppy/vue              |   0.4.7 |
| @uppy/drop-target      |   1.1.3 | @uppy/robodog          |   2.5.4 |
| @uppy/dropbox          |   2.0.6 | uppy                   |   2.9.4 |
| @uppy/facebook         |   2.0.6 |                        |         |

- @uppy/locales: Plural translation in cs_CZ local (JakubHaladej / transloadit#3666)
- @uppy/vue: Add license field to package.json in @uppy/vue (Tobias Trumm / transloadit#3664)
- meta: Add todo comments (Murderlon)
- @uppy/facebook: refactor to ESM (Antoine du Hamel / transloadit#3653)
- meta: locale-pack: refactor to use more parallel processing (Antoine du Hamel / transloadit#3630)
- @uppy/file-input: refactor to ESM (Antoine du Hamel / transloadit#3652)
- meta: sign requests sent to Transloadit in e2e suite (Antoine du Hamel / transloadit#3656)
- meta: add `VITE_TRANSLOADIT_SECRET` for e2e (Antoine du Hamel)
- meta: Update BACKLOG.md (Artur Paikin)
- @uppy/form: refactor to ESM (Antoine du Hamel / transloadit#3654)
- @uppy/dropbox: refactor to ESM (Antoine du Hamel / transloadit#3651)
- meta: sign requests sent to Transloadit in dev env (Antoine du Hamel / transloadit#3517)
- @uppy/drop-target: refactor to ESM (Antoine du Hamel / transloadit#3648)
- @uppy/core: fix `TypeError` when file was removed (Antoine du Hamel / transloadit#3650)
- @uppy/drag-drop: refactor to ESM (Antoine du Hamel / transloadit#3647)
- meta: update outdated files (Antoine du Hamel / transloadit#3646)
- @uppy/compressor: Set meta on file compression (Camilo Forero / transloadit#3644)
- @uppy/transloadit: improve fetch error handling (Antoine du Hamel / transloadit#3637)
- @uppy/box: refactor to ESM (Antoine du Hamel / transloadit#3643)
- @uppy/utils: Fix getFileType for dicom images (Merlijn Vos / transloadit#3610)
- @uppy/aws-s3-multipart: Add `companionCookiesRule` type to @uppy/aws-s3-multipart (Mauricio Ribeiro / transloadit#3623)
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.

2 participants