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

core improvements: type predicates, keep AxiosResponse & display human errors #12

Merged
merged 3 commits into from May 5, 2021
Merged

core improvements: type predicates, keep AxiosResponse & display human errors #12

merged 3 commits into from May 5, 2021

Conversation

paulRbr
Copy link
Member

@paulRbr paulRbr commented May 4, 2021

This PR is a refactor that contains three improvements. Please see details for each in their commit description:

  • core(api): display invalid definition errors in a more human way
  • core(api): keep AxiosResponse to not loose http response details
  • improvement: learning type predicates in TS function signatures

I recently discovered functions whose return types are a _type
predicate_.

It's really useful to transform types (narrowing down types) when you
check the type is compatible with what you expect.

It adds a bit of clarity in the code by moving the type guards in
dedicated functions.

(more details of type predicates:
https://www.typescriptlang.org/docs/handbook/2/narrowing.html#using-type-predicates)

_Note: the commit also makes use of a second discovered TS feature,
the Type intersection (`&`) to “extend” existing types with more
attributes and make them compatible for narrowing. ✨_
The current response interceptor was keeping only the response body of
every http API calls. However it's not what we want. We want to be
able to check response bodies but also response status for instance.

In this commit such need is not very visible (because we don't read
any response status) but for later changes it will be good, especially
for error handling.

The good thing about this change is that it removes the `any` type I
had to use to “trick” axios. ☺️
Copy link
Member

@scharrier scharrier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left a lot of comments, part of them are due to my unknowledge of Typescript, sorry for those. It seems to be really good moves, congrats on this refactoring!

src/api/error.ts Outdated Show resolved Hide resolved
src/api/error.ts Outdated Show resolved Hide resolved
src/api/error.ts Show resolved Hide resolved
src/definition.ts Show resolved Hide resolved
src/definition.ts Show resolved Hide resolved
src/api/index.ts Show resolved Hide resolved
src/commands/preview.ts Show resolved Hide resolved
This is changing the way we display known invalid definition server
errors. It's good to make the presentation of known possible errors
more presentable to humans.
@paulRbr paulRbr requested a review from scharrier May 4, 2021 14:38
Copy link
Member

@scharrier scharrier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left a final comment on naming, but I think that it's out of the scope of this PR. But, if you agree with me, maybe it would make sense to change it in the upcoming days?

Besides this point, LGTM.

@paulRbr paulRbr merged commit 5b7002d into bump-sh:main May 5, 2021
@paulRbr paulRbr deleted the core-improvements branch May 5, 2021 09:21
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

2 participants