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

Feature/trn 28/improve network errors #83

Merged
merged 8 commits into from
Dec 16, 2020

Conversation

krampstudio
Copy link
Contributor

@krampstudio krampstudio commented Dec 7, 2020

Related to https://oat-sa.atlassian.net/browse/TRN-28

  • introduce new error types (extended using classes to extend native types according to the coding guide)
  • use the error types TimeoutError, ApiError and NetworkError in core/fetchRequest

How to test:

@krampstudio krampstudio marked this pull request as draft December 8, 2020 08:19
@krampstudio krampstudio marked this pull request as ready for review December 15, 2020 11:05
Copy link
Contributor

@oatymart oatymart left a comment

Choose a reason for hiding this comment

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

Code review only, I'll test a bit later.

Question from StackOverflow: since we're compiling with Babel, do we need to have babel-plugin-transform-builtin-extend to make this work?

src/core/error/ApiError.js Show resolved Hide resolved
src/core/error/AuthError.js Outdated Show resolved Hide resolved
/**
* Instantiate an error
* @param {string} message- the error message
* @param {number} errorCode - the HTTP status or custom error code
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks fairly standard, but isn't this mixing 2 concepts? If TAO would one day have custom error codes, such as E42 - bad QTI response format, should they be stored in the same field which can be 500 - Server Error?

Suggestion: keep errorCode and httpStatusCode apart.

Copy link
Contributor Author

@krampstudio krampstudio Dec 15, 2020

Choose a reason for hiding this comment

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

Yes, I think it's the reason why we use errorCode and not status. The status is already available in response.status

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok then is the comment accurate for the intended usage? (the HTTP status or custom error code)

.catch(err => {
if(!err.type){
//offline, CORS, etc.
return Promise.reject(new NetworkError(err.message, 0));
Copy link
Contributor

Choose a reason for hiding this comment

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

Thought: what does errorCode: 0 mean?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://fetch.spec.whatwg.org/#concept-network-error 0 means a network error: the request has not reached the server for various reason (no internet, no route, not allowed by CORS, etc.)

@krampstudio
Copy link
Contributor Author

Code review only, I'll test a bit later.

Question from StackOverflow: since we're compiling with Babel, do we need to have babel-plugin-transform-builtin-extend to make this work?

it looks like the current version of Babel handles it correctly, ie instanceof seems to be working as well as stack trace, but I didn't test on IE

Copy link
Contributor

@shaveko shaveko left a comment

Choose a reason for hiding this comment

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

Looks good, please check the comments on updates/suggestions

src/core/error/ApiError.js Outdated Show resolved Hide resolved
src/core/error/ApiError.js Show resolved Hide resolved

if (Error.captureStackTrace) {
Error.captureStackTrace(this, ApiError);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I've seen in other implementations fallback to this.stack = (new Error()).stack if Error.captureStackTrace is not supported

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Error.captureStackTrace is only used in node.js (so the stack trace appears in the unit tests when failing).
By default, the stack will be captured by inheriting from the Error type, it is generated by the constructor. The implementation should be identical cross browser once handled by Babel.

@krampstudio
Copy link
Contributor Author

After some research on the question of custom error https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Error#Custom_Error_Types. The main point is to ensure extra parameters to the parent constructor because some browsers will have a stack parameter, some will have a lineNumber & fileName.
I have tested it on Chrome, Firefox and Edge. Can one of you test it on Safari ?

Copy link
Contributor

@oatymart oatymart left a comment

Choose a reason for hiding this comment

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

Tests are passing and it's working integrated to https://github.com/oat-sa/tao-deliver-testrunner-nui-fe/pull/102

  • New code is covered by tests (if applicable)
  • Tests are running successfully (old and new ones) on my local machine (if applicable)
  • New code is respecting code style rules
  • New code is respecting best practices
  • New code is not subject to concurrency issues (if applicable)
  • Feature is working correctly on my local machine (if applicable)
  • Acceptance criteria are respected
  • Pull request title and description are meaningful
  • Pull request's target is not master

Copy link
Contributor

@svoronuk svoronuk left a comment

Choose a reason for hiding this comment

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

  • New code is covered by tests (if applicable)
  • Tests are running successfully (old and new ones) on my local machine (if applicable)
  • New code is respecting code style rules
  • New code is respecting best practices
  • New code is not subject to concurrency issues (if applicable)
  • Feature is working correctly on my local machine (if applicable)
  • Acceptance criteria are respected
  • Pull request title and description are meaningful

* @param {boolean} [recoverable=true] - can the user recover after having such error ?
* @param {...} params - additional error parameters (line, etc.)
*/
constructor(message, errorCode, response, recoverable = true, ...params) {
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks correct API-wise, but since there is no standard, I didn't see how we could ever benefit from the extra ...params passed through.

As you said, in Firefox they map onto properties like lineNumber, fileName, but it's proprietary, Safari doesn't do anything with such params.

@oatymart oatymart self-assigned this Dec 16, 2020
@oatymart oatymart merged commit 705fe58 into develop Dec 16, 2020
@oatymart oatymart deleted the feature/TRN-28/improve-network-errors branch December 16, 2020 13:45
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.

4 participants