-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Increment version for new core packages, fix linter errors #13691
Conversation
@@ -39,6 +39,8 @@ function isArrayBuffer(body: any): body is ArrayBuffer | ArrayBufferView { | |||
class ReportTransform extends Transform { | |||
private loadedBytes = 0; | |||
private progressCallback: (progress: TransferProgressEvent) => void; | |||
|
|||
// eslint-disable-next-line @typescript-eslint/ban-types | |||
_transform(chunk: string | Buffer, _encoding: string, callback: Function): void { |
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.
Would that work instead of disabling the rule?
_transform(chunk: string | Buffer, _encoding: string, callback: Function): void { | |
_transform(chunk: string | Buffer, _encoding: string, callback: (error?: any) => void): void { |
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.
Here we are overriding a method from the parent class which we do not own. So, thought it best to not change the signature compared to the same method in the parent class
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.
Function
may have came from the definition of _transform in base.d.ts.
Would (error?: Error) => void
be better? The doc says it must be an Error
object if an error occurred.
@@ -127,7 +129,7 @@ export class NodeHttpsClient implements HttpsClient { | |||
request | |||
}; | |||
|
|||
let responseStream = shouldDecompress ? getDecodedResponseStream(res, headers) : res; | |||
responseStream = shouldDecompress ? getDecodedResponseStream(res, headers) : res; |
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.
@xirzec This causes change in behavior. The responseStream
used in the catch block was never assigned to anything. Am assuming this is what you had originally wanted
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.
Looks like I didn't catch this problem when I reviewed #12038 -- nice find!
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.
LGTM! 👍
@@ -127,7 +129,7 @@ export class NodeHttpsClient implements HttpsClient { | |||
request | |||
}; | |||
|
|||
let responseStream = shouldDecompress ? getDecodedResponseStream(res, headers) : res; | |||
responseStream = shouldDecompress ? getDecodedResponseStream(res, headers) : res; |
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.
Looks like I didn't catch this problem when I reviewed #12038 -- nice find!
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.
LGTM
Update readme.md (Azure#13691)
Among other things this PR is a replacement for #13615 after including changes from running
rush update --recheck
.Rest of the changes are to fix linter errors in core-client and core-https packages, and enabling further regressions to break the build
Related to #10775