Skip to content

Conversation

@rhodgkins
Copy link

We've been using this library (npm version v1.0.0) in production and had a few occurrences of:

TypeError: Cannot use 'in' operator to search for 'error' in undefined
at Function.ResponseHandler.ParseError (ResponseHandler.js:20)
at Function.ResponseHandler.init (ResponseHandler.js:14)
at (/app/node_modules/@microsoft/microsoft-graph-client/lib/src/GraphRequest.js:191)

I've changed the check to ensure the in operator is only being used against an object.

We've been using this library (npm version v1.0.0) in production and had a few occurrences of:

```
TypeError: Cannot use 'in' operator to search for 'error' in undefined
at Function.ResponseHandler.ParseError (ResponseHandler.js:20)
at Function.ResponseHandler.init (ResponseHandler.js:14)
at (/app/node_modules/@microsoft/microsoft-graph-client/lib/src/GraphRequest.js:191)
```

I've changed the check to ensure the `in` operator is only being used against an object.
@MIchaelMainer MIchaelMainer requested a review from bullsseye May 24, 2018 16:48
var errObj;
if (!('rawResponse' in rawErr)) {
if (rawErr.response !== undefined && rawErr.response.body !== null && 'error' in rawErr.response.body) {
if (rawErr.response !== undefined && rawErr.response.body !== null && typeof rawErr.response.body === 'object' && 'error' in rawErr.response.body) {
Copy link
Contributor

Choose a reason for hiding this comment

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

should not it be better to check with the following condition
(rawErr.response !== undefined && rawErr.response.body !== undefined && rawErr.response.body !== null && 'error' in rawErr.response.body)
rather than finding the type and verifying it?

Copy link
Author

Choose a reason for hiding this comment

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

The error property needs to be an object anyway so it might as well be checked.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes it makes sense.

Copy link
Contributor

@bullsseye bullsseye left a comment

Choose a reason for hiding this comment

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

The same change needs to be done in corresponding ts file and compile it before pushing

@muthurathinam
Copy link
Contributor

ResponseHandler is refactored with the FetchAPI update PR and the similar GraphError handling bug is fixed in this PR, Since this PR is no longer valid so closing.!!

@rhodgkins Appreciate your time and effort for raising this pull request.

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.

3 participants