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

Terminate method doesnt execute options method onBeforeRequest #209

Closed
walterdavalos opened this issue Jul 15, 2020 · 3 comments
Closed

Terminate method doesnt execute options method onBeforeRequest #209

walterdavalos opened this issue Jul 15, 2020 · 3 comments
Labels
bug

Comments

@walterdavalos
Copy link

Describe the bug
When I invoke the method upload.abort(true) i noticed a DELETE request that did not send the parameters that I specified in the options method onBeforeRequest.

To Reproduce
Steps to reproduce the behavior:

  1. Set for example onBeforeRequest to add a custom header
  2. Start upload
  3. Abort upload with parameter shoultTerminate set true: upload.abort(true)
  4. Check if the request has the custom header

Setup details

  • Runtime environment: Node.js
  • Used tus-js-client version: 2.1.1
@walterdavalos walterdavalos changed the title Terminate method desnt execute options method onBeforeRequest Terminate method doesnt execute options method onBeforeRequest Jul 15, 2020
@Acconut
Copy link
Member

Acconut commented Jul 15, 2020

Yes, you are right, this is missing. The problem is that the request handlers are not invoked, in the terminating function:

tus-js-client/lib/upload.js

Line 118 in 2cad02a

const promise = req.send();

Would you be willing to open a PR for this?

bedgerotto added a commit to bedgerotto/tus-js-client that referenced this issue Aug 20, 2020
@bedgerotto
tus#209 Add onBeforeRequest and onAfterResponse to terminate method
ffe21e2
@bedgerotto
Copy link
Contributor

I created a PR changing the terminate method to call onBeforeRequest and onAfterResponse from options, as it happens in the private method _sendRequest. Hope it can be helpfull

Acconut added a commit that referenced this issue Aug 31, 2020
@bedgerotto @kbs-odd
@Mentthew @dragoscirjan @Acconut @dependabot @kvz
Add onBeforeRequest and onAfterResponse to terminate method (#216)
9b57f0a 
* #209 Add onBeforeRequest and onAfterResponse to terminate method

* Call onAfterResponse even if the request was not successfull

* Correct type definition for Upload#terminate (#217)

inconsistency between typescript typing and function definition

* Allow Promise return type for onBeforeRequest and onAfterResponse. (#212)

* Allow Promise return type for onBeforeRequest and onAfterResponse.

* Allow Promise return type for onBeforeRequest and onAfterResponse: documentation and test case.

* Allow to specify options for Node's request method (#203)

* adding nodejs HttpStack request options

* fix

* fix

* fix

* visual fix

* proposal for request options

* disabling browser

* fix

* Clarify how to import using ESM syntax

* Added onShouldRetry callback for controlling retries (#198)

* Added onShouldRetry callback for controlling retry

Whenever the library is about to retry an upload due to an
error, the new optional callback onShouldRetry will be
called when defined. Its return value will tell the library whether
to actually retry the upload or fail with an error, for example
based on status code checks.

This makes it possible to customize the behavior like reacting on
specific status codes.

* Move onShouldRetry example to usage.md

* Refactored shouldRetry logic

- isOnline is now excluded from the check, so the onShouldRetry callback
  should manually add an online check if desired
- removed inline function and made the conditions more readable
- clarified inline comments about status code check

* Add test assertion for onShouldRetry arguments

* Update usage.md

* Update test-common.js

* Update api.md

Co-authored-by: Marius <marius.kleidl@gmail.com>

* making property 'private', adding small test for 'insecure' request

* fixes ?

* fix: import

* Update js-base64 to a version which does not use eval() anymore

Closes #147

* PR comments fixes

* fixed exports on node, addex HttpStack export on browser

* added new test case - node 14

* test fixes

* test fixes

* fix tests

* Bump lodash from 4.17.14 to 4.17.19

Bumps [lodash](https://github.com/lodash/lodash) from 4.17.14 to 4.17.19.
- [Release notes](https://github.com/lodash/lodash/releases)
- [Commits](lodash/lodash@4.17.14...4.17.19)

Signed-off-by: dependabot[bot] <support@github.com>

* Clean up request initialization and add proper test

Co-authored-by: Dragos Cirjan <dragos.cirjan@cellebrite.com>
Co-authored-by: Dragos Cirjan <dragos.cirjan@gmail.com>
Co-authored-by: Marius <marius@transloadit.com>
Co-authored-by: Vincent Petry <pvince81@owncloud.com>
Co-authored-by: Marius <marius.kleidl@gmail.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Kevin van Zonneveld <vanzonneveld@gmail.com>

* Bump elliptic from 6.3.1 to 6.5.3 (#214)

Bumps [elliptic](https://github.com/indutny/elliptic) from 6.3.1 to 6.5.3.
- [Release notes](https://github.com/indutny/elliptic/releases)
- [Commits](indutny/elliptic@v6.3.1...v6.5.3)

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Update dependencies using 'npm audit fix'

* Do not pass URL as separate parameter

This is not supported in Node.js 8 and 9.

* Downgrade tsd to support Node.js 8 and 9

See https://github.com/SamVerschueren/tsd/releases/tag/v0.12.1

* Correct type definition for Upload#terminate (#217)

inconsistency between typescript typing and function definition

* Minor cleanups

Co-authored-by: Dragos Cirjan <dragos.cirjan+github2@gmail.com>
Co-authored-by: Dragos Cirjan <dragos.cirjan@cellebrite.com>
Co-authored-by: Dragos Cirjan <dragos.cirjan@gmail.com>
Co-authored-by: Marius <marius@transloadit.com>
Co-authored-by: Vincent Petry <pvince81@owncloud.com>
Co-authored-by: Marius <marius.kleidl@gmail.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Kevin van Zonneveld <vanzonneveld@gmail.com>
Co-authored-by: kabaliserv <66329463+kabaliserv@users.noreply.github.com>

* Allow to return Promises in onBeforeRequest for terminate

Co-authored-by: kabaliserv <66329463+kabaliserv@users.noreply.github.com>
Co-authored-by: Mentthew <34655639+Mentthew@users.noreply.github.com>
Co-authored-by: Dragos Cirjan <dragos.cirjan+github2@gmail.com>
Co-authored-by: Dragos Cirjan <dragos.cirjan@cellebrite.com>
Co-authored-by: Dragos Cirjan <dragos.cirjan@gmail.com>
Co-authored-by: Marius <marius@transloadit.com>
Co-authored-by: Vincent Petry <pvince81@owncloud.com>
Co-authored-by: Marius <marius.kleidl@gmail.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Kevin van Zonneveld <vanzonneveld@gmail.com>
@Acconut
Copy link
Member

Acconut commented Sep 1, 2020

Fixed in #216.

@Acconut Acconut closed this as completed Sep 1, 2020
@snyk-bot snyk-bot mentioned this issue Jul 28, 2021
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@Acconut @bedgerotto @walterdavalos