Improved APIError Message #38
Conversation
src/api/client.ts
Outdated
| } else if ( | ||
| error.what && | ||
| error.what.length > 0 && | ||
| error.details[0].message && |
There was a problem hiding this comment.
We should guard that error.details exist and has at least one item here
| } else if (error.what && error.what.length > 0) { | ||
| // lacks detailed message, fallback to error statement | ||
| // note this isn't very helpful to developers | ||
| return error.what |
There was a problem hiding this comment.
A test case that hits this would be good to ensure that the above if statement doesn't cause issues (make coverage will generate and open a test coverage report for you)
test/api.ts
Outdated
| signatures: [signature], | ||
| }) | ||
| const result = await jungle.v1.chain.push_transaction(signedTransaction) | ||
| } catch (error) { |
There was a problem hiding this comment.
This test case silently passes if no error is thrown, add an assert.fail('expected error') above or refactor to use assert.rejects
| @@ -424,8 +527,11 @@ suite('api v1', function () { | |||
| '03ef96a276a252b66595d91006ad0ff38ed999816f078bc5d87f88368a9354e7' | |||
| ) | |||
| assert(Array.isArray(res.traces), 'response should have traces') | |||
| assert.equal(res.id, '03ef96a276a252b66595d91006ad0ff38ed999816f078bc5d87f88368a9354e7') | |||
| assert.equal(res.block_num, 199068081) | |||
There was a problem hiding this comment.
need to look at why these started to fail. fixes get tests to pass.
|
Investigating tracking down one item before ready to submit. get_transaction now requiring type conversion in tests
|
| @@ -40,7 +40,8 @@ | |||
| "@types/node": "^18.6.5", | |||
| "@typescript-eslint/eslint-plugin": "^5.4.0", | |||
| "@typescript-eslint/parser": "^5.4.0", | |||
| "chai": "^4.3.4", | |||
| "chai": "^4.3.6", | |||
| "chai-as-promised": "^7.1.1", | |||
There was a problem hiding this comment.
Try not to introduce new dependencies, chai is only used as an assert polyfill for browser tests and we don't use "expect" style testing
| return error.details[0].message | ||
| } else if (error.what && error.what.length > 0) { | ||
| } else if (error?.what?.length > 0 && error?.details?.[0]?.message?.length > 0) { |
There was a problem hiding this comment.
Avoid using existential operators (?), we have had issues with downstream libraries and applications not being able to import our library due to those and had to scrub them from the codebase
There was a problem hiding this comment.
No problem, do you have a suggestion on how to proceed? Optional values is the root issue because, detecting undefined | null | '' strings forced the negative tests for code coverage. If the code was restructured to never allow falsy values the optionals along with the negative tests could go away. No negative tests, no need for chai.except. Happy to discuss if you want to run through the options. If you know what you want just let me know.
There was a problem hiding this comment.
Not sure I follow, can't you just expand ? by doing (error != undefined && error.what != undefined) etc?
|
Closing this pull request. Moving the discussion on best path forward back to issue #32 |
Added internal nodeos message returned to clients when API errors are returned. Fixes #32