Modernization and error handling#95
Conversation
…orting [codex] harden network error reporting
There was a problem hiding this comment.
Pull request overview
This PR modernizes the SDK’s build/test toolchain (Node/Jest/TS), improves resilience of network response parsing and error reporting, and hardens request URL construction by encoding path segments.
Changes:
- Centralized URL construction with per-segment
encodeURIComponentto avoid unsafe/unintended path injection. - Made network response parsing tolerant of empty/non-JSON bodies and added richer JSON-parse error reporting via
QonversionError.responseCode. - Upgraded Node/tooling dependencies and updated CI workflows and unit tests accordingly.
Reviewed changes
Copilot reviewed 30 out of 33 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tsconfig.json | Adds Node typings, enables skipLibCheck, and excludes integration tests from compilation. |
| sdk/src/internal/network/RequestConfigurator.ts | Introduces buildUrl helper to encode path segments for safer request URLs. |
| sdk/src/internal/network/NetworkClient.ts | Reworks response body parsing (empty body, non-JSON, malformed JSON → structured error). |
| sdk/src/internal/network/ApiInteractor.ts | Hardens error extraction from unexpected payloads and refines retry behavior for execution errors. |
| sdk/src/exception/QonversionError.ts | Extends error shape with details and responseCode to improve reporting/handling. |
| sdk/src/tests/internal/utils/DelayedWorker.test.ts | Updates deprecated Jest call assertions (toBeCalled → toHaveBeenCalled). |
| sdk/src/tests/internal/userProperties/UserPropertiesStorage.test.ts | Updates Jest call assertions. |
| sdk/src/tests/internal/userProperties/UserPropertiesService.test.ts | Updates Jest call assertions. |
| sdk/src/tests/internal/userProperties/UserPropertiesController.test.ts | Updates Jest call assertions. |
| sdk/src/tests/internal/user/UserServiceDecorator.test.ts | Fixes promise usage to resolve deterministically and updates assertions. |
| sdk/src/tests/internal/user/UserService.test.ts | Updates Jest call assertions. |
| sdk/src/tests/internal/user/UserIdGenerator.test.ts | Updates Jest call assertions. |
| sdk/src/tests/internal/user/UserDataStorage.test.ts | Updates Jest call assertions. |
| sdk/src/tests/internal/user/UserController.test.ts | Updates Jest call assertions. |
| sdk/src/tests/internal/user/IdentityService.test.ts | Updates Jest call assertions. |
| sdk/src/tests/internal/purchases/PurchasesService.test.ts | Updates Jest call assertions. |
| sdk/src/tests/internal/purchases/PurchasesController.test.ts | Updates Jest call assertions. |
| sdk/src/tests/internal/network/RequestConfigurator.test.ts | Adjusts expected URLs for encoded segments; adds encoding-focused test cases. |
| sdk/src/tests/internal/network/NetworkClient.test.ts | Adds coverage for empty/plain-text/malformed JSON response parsing and error details. |
| sdk/src/tests/internal/network/ApiInteractor.test.ts | Adds coverage for parse-error retry behavior and malformed/plain-text error payload handling. |
| sdk/src/tests/internal/logger/logger.test.ts | Updates Jest call assertions. |
| sdk/src/tests/internal/entitlements/EntitlementsService.test.ts | Updates Jest call assertions. |
| sdk/src/tests/internal/entitlements/EntitlementsController.test.ts | Updates Jest call assertions. |
| sdk/src/tests/internal/common/LocalStorage.test.ts | Updates Jest call assertions. |
| sdk/src/tests/internal/QonversionInternal.test.ts | Updates Jest call assertions and promise-return expectations. |
| sdk/src/tests/Qonversion.test.ts | Updates Jest call assertions. |
| package.json | Upgrades dev tooling (Jest/Babel/TS/uuid/etc.). |
| .yarnrc.yml | Adds Yarn config (node-modules linker). |
| .gitignore | Ignores .yarn/ directory. |
| .github/workflows/publish.yml | Moves publish workflow to Node 22 and enables Yarn caching. |
| .github/workflows/pr-checks.yml | Moves PR checks to Node 22 and enables Yarn caching. |
| .github/workflows/integration_tests.yml | Moves scheduled integration tests to Node 22 and enables Yarn caching. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| - name: Use Node.js | ||
| uses: actions/setup-node@v4 | ||
| with: | ||
| node-version: '20.x' | ||
|
|
||
| - name: Cache NPM # leverage npm cache on repeated workflow runs if package.json didn't change | ||
| uses: actions/cache@v4 | ||
| with: | ||
| path: ~/.npm | ||
| key: ${{ runner.os }}-node-${{ hashFiles('**/package-lock.json') }} | ||
| restore-keys: | | ||
| ${{ runner.os }}-node- | ||
| node-version: '22.x' | ||
| cache: 'yarn' | ||
| cache-dependency-path: yarn.lock | ||
| - name: Install Dependencies | ||
| run: yarn |
There was a problem hiding this comment.
pr-checks.yml sets defaults.run.working-directory: ./sdk, but there is no package.json under ./sdk in this repo. This will fail when the runner uses Yarn classic (v1), which is the default unless you explicitly enable Corepack/pin Yarn. Consider either running Yarn from the repo root (remove the working-directory default), or pinning Yarn via packageManager + corepack enable so Yarn can resolve the project root from subdirectories reliably.
| "devDependencies": { | ||
| "@babel/core": "^7.18.2", | ||
| "@babel/preset-env": "^7.18.2", | ||
| "@babel/preset-typescript": "^7.17.12", | ||
| "@types/jest": "^29.4.0", | ||
| "@typescript-eslint/eslint-plugin": "^5.28.0", | ||
| "@typescript-eslint/parser": "^5.28.0", | ||
| "babel-jest": "^28.1.0", | ||
| "dotenv": "^16.3.1", | ||
| "eslint": "^8.17.0", | ||
| "eslint-config-prettier": "^8.5.0", | ||
| "jest": "^29.4.2", | ||
| "ts-jest": "^29.1.0", | ||
| "typescript": "^4.7.3" | ||
| "@babel/core": "^7.29.0", | ||
| "@babel/preset-env": "^7.29.2", | ||
| "@babel/preset-typescript": "^7.28.5", | ||
| "@types/jest": "^30.0.0", |
There was a problem hiding this comment.
This PR introduces Yarn Berry config (.yarnrc.yml) and updates workflows to rely on Yarn caching, but package.json still doesn’t pin a Yarn version via the packageManager field. That can lead to CI/dev using different Yarn majors (e.g., runner’s Yarn v1 vs local Yarn Berry), producing inconsistent installs. Consider adding packageManager: "yarn@<version>" (and enabling Corepack in CI).
I noticed that this repository hasn't seen any updates in a couple of years, and I ran into issues wit my own projects with regards to error handling that needed to be addressed.