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

fix: Throw error if graphql response field is null #59

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

stergion
Copy link

Some times a graphql response fields have value null. This causes currentValue.hasOwnProperty(searchProp) to throw TypeError: Cannot read properties of null (reading 'hasOwnProperty') error. The fix checks first if currentValue is null before calling hasOwnProperty() method.

Resolves #58


Behavior

Before the change?

  • Throughs TypeError if graphql response field has value of null.

After the change?

  • Checks if field is null to prevent TypeError

Some times a graphql response fields have value null. This causes currentValue.hasOwnProperty(searchProp) to throw `TypeError: Cannot read properties of null (reading 'hasOwnProperty')` error. The fix checks first if `currentValue` is null before calling hasOwnProperty() method.
@ghost ghost added this to Inbox in JS Feb 12, 2023
@wolfy1339 wolfy1339 added the Type: Bug Something isn't working as documented label Feb 12, 2023
@ghost ghost moved this from Inbox to Bugs in JS Feb 12, 2023
@kfcampbell
Copy link
Member

Some times a graphql response fields have value null.

Are you saying this is intended behavior and not an API issue?

@stergion
Copy link
Author

I'm not sure if this is an API issue or not, although it seems like it.
Some fields that I have checked, like name, location, bio, that return null are of type String, so returning an empty string should be the better option.

Isn't it better though checking for null anyway?
These were just the fields that I found. There might be many more fields that return null and some may not be able to change them.

@nickfloyd
Copy link
Contributor

Hey @stergion,

Thanks for doing this ❤️ . Would you mind writing some test coverage for the null check as we'll? That should allow future contributors to understand why it was put in place and also makes sure we don't inadvertently break things in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Bug Something isn't working as documented
Projects
Status: 🏗 In progress
JS
  
Bugs
Development

Successfully merging this pull request may close these issues.

[BUG]: deepFindPathToProperty() throws TypeError if graphql response field is null
4 participants