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

refactor: improved error handling and error types #226

Merged
merged 9 commits into from
Mar 1, 2023

Conversation

cdaringe
Copy link
Contributor

@cdaringe cdaringe commented Feb 8, 2023

problem

Error handling is not consistent or helpful in various cases, emitting generic and unhelpful messages. See #225

solution

The current strategy is to funnel all possible failure modes in the HTTP client down through the handleError function. This pattern exists because there is are adapters to a generic HTTP client interface, but adapters do not need to map any error cases in. To rework that pattern is a massive refactor, likely out of scope of this PR.

Thus, the approach taken here is to map failure types into expected failure types, and match (non-exhaustively) on those failure types to extract relevant information. This approach is nice because it's simple. It's a bit of a leaky abstraction, because now handlerError must know about all error types. An improved implementation later could be having Error type mappings in the HTTP client interface and the adapters themselves.

The solution does the following:

  • update adapters to throw well known Error types
  • update the single error handler to inspect for those error types, and try to do more surgical extraction of well known crowdin API datas

todo

  • Tests. I don't want to put a pattern in here for testing this without maintainer guidance. tell me how you want tests structured for this type of thing. ideally, the maintainers would stub in a test or two!

jest.config.js Show resolved Hide resolved
jest.config.js Show resolved Hide resolved
package.json Show resolved Hide resolved
@cdaringe
Copy link
Contributor Author

cdaringe commented Feb 8, 2023

most all current tests passed... not sure how to get info from these "Basic" checks. clicking the links end up all being self referential 😵

@andrii-bodnar
Copy link
Member

@cdaringe thanks a lot for your contribution! 🚀

Here is the error log for the failed case:

> @crowdin/crowdin-api-client@1.21.1 test-coverage /home/vsts/work/1/s
> jest --config jestconfig.json --ci --reporters=jest-junit --reporters=default --coverage --coverageReporters=cobertura --coverageReporters=html

Error: Can't find a root directory while resolving a config file path.
Provided path to resolve: jestconfig.json
cwd: /home/vsts/work/1/s
    at resolveConfigPath (/home/vsts/work/1/s/node_modules/jest-config/build/resolveConfigPath.js:134:11)
    at readConfig (/home/vsts/work/1/s/node_modules/jest-config/build/index.js:220:49)
    at readConfigs (/home/vsts/work/1/s/node_modules/jest-config/build/index.js:420:32)
    at runCLI (/home/vsts/work/1/s/node_modules/@jest/core/build/cli/index.js:133:29)
    at Object.run (/home/vsts/work/1/s/node_modules/jest-cli/build/cli/index.js:155:62)
    at Object.<anonymous> (/home/vsts/work/1/s/node_modules/jest-cli/bin/jest.js:16:17)
    at Module._compile (internal/modules/cjs/loader.js:999:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1027:10)
    at Module.load (internal/modules/cjs/loader.js:863:32)
    at Function.Module._load (internal/modules/cjs/loader.js:708:14)

@andrii-bodnar
Copy link
Member

Related to #208

Copy link
Collaborator

@yevheniyJ yevheniyJ left a comment

Choose a reason for hiding this comment

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

Overall looks very good and thanks for contribution. Left few comments.

src/core/index.ts Outdated Show resolved Hide resolved
src/core/internal/fetch/fetchClient.ts Outdated Show resolved Hide resolved
src/core/internal/fetch/fetchClient.ts Outdated Show resolved Hide resolved
@andrii-bodnar
Copy link
Member

Hi @cdaringe, could you please address the review comments? Let us know if you need any assistance in order to finish this PR 🙂

@cdaringe
Copy link
Contributor Author

got some tests goin too: 8ce133c

@cdaringe cdaringe requested a review from yevheniyJ February 22, 2023 21:25
package.json Outdated Show resolved Hide resolved
src/core/index.ts Outdated Show resolved Hide resolved
Copy link
Collaborator

@yevheniyJ yevheniyJ 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 to me! Thanks a lot

@andrii-bodnar andrii-bodnar linked an issue Feb 28, 2023 that may be closed by this pull request
@andrii-bodnar
Copy link
Member

@cdaringe we're almost ready to merge this PR, could you please check the unresolved comments?

package.json Show resolved Hide resolved
@andrii-bodnar andrii-bodnar merged commit b6b714e into crowdin:master Mar 1, 2023
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.

RFC: Refactor handleError to explicitly accept Error instances
4 participants