Skip to content

Modernization and error handling#95

Open
megastep wants to merge 9 commits into
qonversion:developfrom
catloafsoft:modernization-and-error-handling
Open

Modernization and error handling#95
megastep wants to merge 9 commits into
qonversion:developfrom
catloafsoft:modernization-and-error-handling

Conversation

@megastep
Copy link
Copy Markdown

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.

  • Upgraded several dependencies (including Jest) and build with Node 22 (20 is deprecated)
  • Hardened network error reporting
  • Fixed some potential security issues with URL encoding

Copilot AI review requested due to automatic review settings March 19, 2026 23:53
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

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 encodeURIComponent to 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 (toBeCalledtoHaveBeenCalled).
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.

Comment on lines 17 to 24
- 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
Copy link

Copilot AI Mar 20, 2026

Choose a reason for hiding this comment

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

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.

Copilot uses AI. Check for mistakes.
Comment thread package.json
Comment on lines 38 to +42
"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",
Copy link

Copilot AI Mar 20, 2026

Choose a reason for hiding this comment

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

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).

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants