Improve default connection logic #734
Conversation
kmcginnes
force-pushed
the
bootstrap-app
branch
from
9aec5dc to
7a3aa7b
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #734 +/- ##
==========================================
+ Coverage 14.88% 19.27% +4.38%
==========================================
Files 566 532 -34
Lines 25121 23024 -2097
Branches 1170 1175 +5
==========================================
+ Hits 3739 4437 +698
+ Misses 21050 18319 -2731
+ Partials 332 268 -64
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| expect(actual).toEqual(data); | ||
| }); | ||
|
|
||
| test("should handle missing values", () => { |
There was a problem hiding this comment.
Nit: can consider adding test that invalid/unexpected values in the response result in defaults since the logic uses statements like z.string().url().catch("") and z.boolean().default(false). Ie. what happens if url is not correct format or a boolean value is 'TRUE' instead of true.
There was a problem hiding this comment.
I added a test for invalid URLs. But the booleans and numbers aren't string values, so they should come in fine or the await response.json() will fail, which results in null config, which is appropriate.
I wish it was easier to mock out fetch so I could do more testing. I have a plan for mocking fetch, but it is too involved for this change.
There was a problem hiding this comment.
I ran a test where I forced a string in to the boolean property to see what happens. It fails the Zod validator properly, but I want to improve the logging of that error. So I'm going to update the parsing logic to print out a nicer console error.
There was a problem hiding this comment.
Updated the logging of parse errors
| enabled: isStoreLoaded && configuration.size === 0, | ||
| }); | ||
|
|
||
| const config = defaultConfigQuery.data; |
There was a problem hiding this comment.
Nit: Can config be renamed to something more descriptive as to differentiate it better from configuration? At first glance it's not clear the difference between the two.
There was a problem hiding this comment.
I could, but I left it named config to match the old code to reduce the code changed.
It really should be renamed, so since you brought it up I'll make the change 😄
|
LGTM |
Description
index.tsxand in toAppStatusLoader.NeptuneServiceTypeValidation
Related Issues
Check List
license.
pnpm checksto ensure code compiles and meets standards.pnpm testto check if all tests are passing.Changelog.md.