-
Notifications
You must be signed in to change notification settings - Fork 0
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
SDK-3306: Error class hierarchy #186
Conversation
This introduces the core classes for this library, namely: - `InruptClientError`, the superclass for all Inrupt client related errors, - `ClientHttpError`, the base class for HTTP errors Note that classes for specific HTTP errors (Unauthenticated, Not Found...) are not included yet, and will be added later. `ClientHttpError` extends the base `InruptClientError` by implementing two interfaces, `WithProblemDetails` and `WithErrorResponse`, respectively associated to their type guards `hasProblemDetails` and `hasErrorResponse`. Both the `.problemDetails` and `.response` accessors are marked as read-only, and immutability is enforced at runtime for libraries not using static analysis based on TS. The constructor of `ClientHttpError` takes in a response body and a subset of the native `Response` metadata, instead of taking a simple `Response` instance, because reading the `Response` body is an asynchronous operation, which isn't possible when building an object. It is expected that the parsing will be done by the caller in an async context, and the result will be passed to the `ClientHttpError` constructor, which is not the initial design. For the time being, I'm not doing any submodule exports (e.g. `@inrupt/solid-client-error/http`), but this is something we might eventually consider for tree-shaking.
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
/** | ||
* Superclass of all errors thrown by Inrupt's client libraries. | ||
* | ||
* @since unreleased |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it typical to use unreleased
in this context? I am only wondering about the possibility of forgetting to set this to the right version before release.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's the approach we have taken with our other libraries, so that it's easier to search for on release. However, we also have in other places a checklist item specifically for this, which was missing here: a250e37.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few small suggestions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggested test change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This introduces the core classes for this library, namely:
InruptClientError
, the superclass for all Inrupt client related errors,ClientHttpError
, the base class for HTTP errorsNote that classes for specific HTTP errors (Unauthenticated, Not Found...) are not included yet, and will be added later.
ClientHttpError
extends the baseInruptClientError
by implementing two interfaces,WithProblemDetails
andWithErrorResponse
, respectively associated to their type guardshasProblemDetails
andhasErrorResponse
. Both the.problemDetails
and.response
accessors are marked as read-only, and immutability is enforced at runtime for libraries not using static analysis based on TS.The constructor of
ClientHttpError
takes in a response body and a subset of the nativeResponse
metadata, instead of taking a simpleResponse
instance, because reading theResponse
body is an asynchronous operation, which isn't possible when building an object. It is expected that the parsing will be done by the caller in an async context, and the result will be passed to theClientHttpError
constructor, which is not the initial design.For the time being, I'm not doing any submodule exports (e.g.
@inrupt/solid-client-error/http
), but this is something we might eventually consider for tree-shaking.This PR fixes bug #.
New feature description
Checklist
index.ts
, if applicable.@since
and@example
)..ts
files) are listed in theexports
field inpackage.json
, if applicable..ts
files) are listed in thetypedocOptions.entryPoints
field intsconfig.json
, if applicable.This PR bumps the version to .
Release Steps
npm version -- <major|minor|patch> --no-push
with the decided on version (to prevent the tag from being pushed).CHANGELOG.md
to release the latest the version, and set the release date.release/vX.Y.Z
branchCHANGELOG.md
and the automatically generated changes.The release should have a new tag matching the new version number:
vx.y.z
, and point to the release commit.