Add test for issue #789 showing error message [Object object] #792
Conversation
|
I think this sounds legit. When we have an error from there, we should show something meaningful in the api response. |
…bject] - add new test case reproducing the error
bassettsj
force-pushed
the
test-for-callback
branch
from
04c8df0
to
c30ae92
Compare
| @@ -0,0 +1,11 @@ | |||
| module.exports = function errorToString(err) { | |||
There was a problem hiding this comment.
@matteofigus I wasn't sure where to put this but but it would just see if it has a message attribute that is a string and returns that.
| )}${conf.local | ||
| ? settings.registry.localStaticRedirectorPath | ||
| : ''}${filePath}`, | ||
| `${repository.getComponentPath(componentName, componentVersion)}${ |
There was a problem hiding this comment.
Didn't change this, but it seems prettier seems to want to change it.
There was a problem hiding this comment.
No worries, that is by design. Perhaps we should mention that in the contributing guidelines to avoid confusion :)
|
Closes #789 |
src/utils/error-to-string.js
Outdated
| @@ -0,0 +1,11 @@ | |||
| module.exports = function errorToString(err) { | |||
| const isString = typeof error === 'string'; | |||
There was a problem hiding this comment.
can we use lodash's _.isString? This is for consistency with other bits in the codebase
| return err.msg; | ||
| } | ||
|
|
||
| return err + ''; |
There was a problem hiding this comment.
what about err.toString() which perhaps is a bit more readable
There was a problem hiding this comment.
Hmm... That might cause errors like TypeError: Cannot read property 'toString' of undefined if it is null or undefined.
Please do not create a Pull Request without creating an issue first.
Any change needs to be discussed before proceeding.
Failure to do so may result in the rejection of the pull request.
Please provide enough information so that others can review your pull request:
Explain the details for making this change. What existing problem does the pull request solve?
Demonstrating the bad error message I was seeing after setting up demo registry on heroku. This is showing the problematic error message I saw.
Test plan (required)
Running unit tests should prove if the error is fixed or not.
Closing issues
#789 would be closed if I fixed the failing test as well, but I would like to discuss if it is an actual bad error condition or not.