-
Notifications
You must be signed in to change notification settings - Fork 5.1k
Fix: ensure using this.provider as the context this
#3721
Conversation
this
this
Is it really necessary for the context to be |
Yeah, We've got a problem, and now I have to detect the context whether is the More discussion may check here: #3649 (comment) |
This makes sense but will need to test it out later today and probably add a testcase for this |
Here's a codesandbox with a reproduction of this issue with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this looks good to me, bind
ing provider context is relatively harmless in this situation and good practice for this style code. it's a little strange though because callbackifiy should preserve the this
but maybe due to the changes in the pr mentioned that has change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you consider to write a test to avoid such issues in the future?
Any idea when this will get merged and released with tag 1.3.1 ? |
Update to resolve web3-core-requestmanager issue reported in web3/web3.js#3721 Assume a provider is passed to DydxClient, where the request function uses the 'this' keyword (for example to ensure initialization of the provider, perhaps fetching some data). When calling functions, such as: `client.eth.exchange.deposit(....)` The flow will eventually reach: 'RequestManager.send' In this, when calling the provider.request, we do not bind the provider, thus the value of 'this' is undefined, and any call to 'this' will result in an error due to undefined value. The fix will add the '.bind(this.provider)' which is currently missing, and make the 'this' value to be defined to the provider initially provided to the client.
Update to resolve web3-core-requestmanager issue reported in web3/web3.js#3721 Assume a provider is passed to DydxClient, where the request function uses the 'this' keyword (for example to ensure initialization of the provider, perhaps fetching some data). When calling functions, such as: `client.eth.exchange.deposit(....)` The flow will eventually reach: 'RequestManager.send' In this, when calling the provider.request, we do not bind the provider, thus the value of 'this' is undefined, and any call to 'this' will result in an error due to undefined value. The fix will add the '.bind(this.provider)' which is currently missing, and make the 'this' value to be defined to the provider initially provided to the client.
Description
The bug is brought in this PR: #3649
It may cause some problems when calling
provider.request
. As the contextthis
maybewindow
sometimes, so we should make sure the context is theprovider
.Type of change
Checklist:
npm run dtslint
with success and extended the tests and types if necessary.npm run test:unit
with success.npm run test:cov
and my test cases cover all the lines and branches of the added code.npm run build
and testeddist/web3.min.js
in a browser.CHANGELOG.md
file in the root folder.