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

Better check in getObjectProperty() #2703

Closed
wants to merge 1 commit into from
Closed

Better check in getObjectProperty() #2703

wants to merge 1 commit into from

Conversation

hufftheweevil
Copy link

Added two more checks to ensure that obj is not undefined or null. If either are the case, the function will error without these checks.

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)

Added two more checks to ensure that `obj` is not `undefined` or `null`. If either are the case, the function will error without these checks.
@jsf-clabot
Copy link

jsf-clabot commented Sep 23, 2020

CLA assistant check
Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.

@knolleary
Copy link
Member

Throwing an error is part of the API. This change will break things that expect the exception to be thrown - you can see the unit tests are failing as a result of this PR.

If anything, the docs for the function need updating to explain how/why the exception will be thrown and what it signifies.

@hufftheweevil
Copy link
Author

Got it. Yeah - I think the docs could use some work, all around. Otherwise Node-RED is awesome! Thanks for taking the time to look at this.

@dceejay
Copy link
Member

dceejay commented Sep 23, 2020

we take PRs to docs as well ;-)

@hufftheweevil
Copy link
Author

Where is that at? On https://nodered.org/docs/?

@dceejay
Copy link
Member

dceejay commented Sep 24, 2020

The main docs site project is https://github.com/node-red/node-red.github.io/tree/master/docs

@knolleary
Copy link
Member

Right, but this function isn't documented on the website. I was referring to the doc in the source code for this function.

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.

None yet

4 participants