Skip to content

Commit

Permalink
Type-checking improvements
Browse files Browse the repository at this point in the history
- Running `tsc` with no options checks every file (`build` still builds
  source, like before)
- Use jest's built-in types
- Enable strictest typechecking settings
- The test workflow runs `yarn tsc`
  • Loading branch information
nicknovitski committed Sep 6, 2024
1 parent 8433e8b commit 5fb64fa
Show file tree
Hide file tree
Showing 10 changed files with 279 additions and 176 deletions.
1 change: 1 addition & 0 deletions .github/workflows/tests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ jobs:
path: ${{ github.workspace }}/node_modules
key: ${{ runner.OS }}-${{ matrix.node-version}}-node_modules-${{ hashFiles('yarn.lock') }}
- run: yarn --frozen-lockfile
- run: yarn tsc
- run: yarn lint --max-warnings=0
- run: yarn test --coverage
- name: Create coverage report flag
Expand Down
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -12,3 +12,4 @@ node_modules/

# Build output
/build/
*.tsbuildinfo
12 changes: 0 additions & 12 deletions build.sh

This file was deleted.

5 changes: 3 additions & 2 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
"build"
],
"scripts": {
"build": "./build.sh",
"build": "tsc --project tsconfig.build.json",
"lint": "eslint src",
"prepare": "yarn build",
"test": "jest",
Expand Down Expand Up @@ -44,7 +44,8 @@
"promise-retry": "^2.0.1"
},
"devDependencies": {
"@types/jest": "^29.5.12",
"@tsconfig/node-lts": "^20.1.3",
"@tsconfig/strictest": "^2.0.5",
"@types/node-fetch": "^2.6.11",
"@types/promise-retry": "^1.1.6",
"eslint": "^8.57.0",
Expand Down
30 changes: 16 additions & 14 deletions src/ExpoClient.ts
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ export class Expo {
actualMessagesCount === 1 ? 'ticket' : 'tickets'
} but got ${data.length}`,
);
apiError.data = data;
apiError['data'] = data;
throw apiError;
}

Expand All @@ -126,7 +126,7 @@ export class Expo {
const apiError: ExtensibleError = new Error(
`Expected Expo to respond with a map from receipt IDs to receipts but received data of another type`,
);
apiError.data = data;
apiError['data'] = data;
throw apiError;
}

Expand Down Expand Up @@ -268,7 +268,7 @@ export class Expo {

if (!result.errors || !Array.isArray(result.errors) || !result.errors.length) {
const apiError: ExtensibleError = await this.getTextResponseErrorAsync(response, textBody);
apiError.errorData = result;
apiError['errorData'] = result;
return apiError;
}

Expand All @@ -279,8 +279,8 @@ export class Expo {
const apiError: ExtensibleError = new Error(
`Expo responded with an error with status code ${response.status}: ` + text,
);
apiError.statusCode = response.status;
apiError.errorText = text;
apiError['statusCode'] = response.status;
apiError['errorText'] = text;
return apiError;
}

Expand All @@ -289,29 +289,31 @@ export class Expo {
* contains any other errors.
*/
private getErrorFromResult(response: FetchResponse, result: ApiResult): Error {
assert(result.errors && result.errors.length > 0, `Expected at least one error from Expo`);
const [errorData, ...otherErrorData] = result.errors!;
const error: ExtensibleError = this.getErrorFromResultError(errorData);
const noErrorsMessage = `Expected at least one error from Expo`;
assert(result.errors, noErrorsMessage);
const [errorData, ...otherErrorData] = result.errors;
assert.ok(errorData, noErrorsMessage);
const error = this.getErrorFromResultError(errorData);
if (otherErrorData.length) {
error.others = otherErrorData.map((data) => this.getErrorFromResultError(data));
error['others'] = otherErrorData.map((data) => this.getErrorFromResultError(data));
}
error.statusCode = response.status;
error['statusCode'] = response.status;
return error;
}

/**
* Returns an error for a single API error
*/
private getErrorFromResultError(errorData: ApiResultError): Error {
private getErrorFromResultError(errorData: ApiResultError): ExtensibleError {
const error: ExtensibleError = new Error(errorData.message);
error.code = errorData.code;
error['code'] = errorData.code;

if (errorData.details != null) {
error.details = errorData.details;
error['details'] = errorData.details;
}

if (errorData.stack != null) {
error.serverStack = errorData.stack;
error['serverStack'] = errorData.stack;
}

return error;
Expand Down
2 changes: 1 addition & 1 deletion src/ExpoClientValues.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
* The EXPO_BASE_URL environment variable is only for internal Expo use
* when testing the push service locally.
*/
const baseUrl = process.env.EXPO_BASE_URL ?? 'https://exp.host';
const baseUrl = process.env['EXPO_BASE_URL'] ?? 'https://exp.host';

export const sendApiUrl = `${baseUrl}/--/api/v2/push/send`;

Expand Down
54 changes: 29 additions & 25 deletions src/__tests__/ExpoClient-test.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
import { jest, afterEach, beforeEach, describe, test, expect } from '@jest/globals';
import fetch from 'node-fetch';
import assert from 'node:assert';

import ExpoClient, { ExpoPushMessage } from '../ExpoClient';
import { getReceiptsApiUrl, sendApiUrl } from '../ExpoClientValues';
Expand Down Expand Up @@ -311,8 +313,8 @@ describe('chunking push notification messages', () => {
const client = new ExpoClient();
const messages = new Array(10).fill({ to: '?' });
const chunks = client.chunkPushNotifications(messages);
expect(chunks.length).toBe(1);
expect(chunks[0].length).toBe(10);
expect(chunks).toHaveLength(1);
expect(chunks[0]).toHaveLength(10);
});

test('chunks single push notification message with lists of recipients', () => {
Expand All @@ -323,7 +325,7 @@ describe('chunking push notification messages', () => {
const chunks = client.chunkPushNotifications(messages);
for (const chunk of chunks) {
// Each chunk should only contain a single message with 100 recipients
expect(chunk.length).toBe(1);
expect(chunk).toHaveLength(1);
}
const totalMessageCount = countAndValidateMessages(chunks);
expect(totalMessageCount).toBe(messagesLength);
Expand All @@ -335,9 +337,10 @@ describe('chunking push notification messages', () => {
const client = new ExpoClient();
const messages = [{ to: new Array(messagesLength).fill('?') }];
const chunks = client.chunkPushNotifications(messages);
expect(chunks.length).toBe(1);
expect(chunks[0].length).toBe(1);
expect(chunks[0][0].to.length).toBe(messagesLength);
expect(chunks).toHaveLength(1);
expect(chunks[0]).toHaveLength(1);
assert.ok(chunks[0]);
expect(chunks[0][0]?.to).toHaveLength(messagesLength);
});

test('chunks push notification messages mixed with lists of recipients and single recipient', () => {
Expand All @@ -362,69 +365,70 @@ describe('chunking a single push notification message with multiple recipients',
test('one message with 100 recipients', () => {
const messages = [{ to: new Array(100).fill('?') }];
const chunks = client.chunkPushNotifications(messages);
expect(chunks.length).toBe(1);
expect(chunks[0].length).toBe(1);
expect(chunks[0][0].to.length).toBe(100);
expect(chunks).toHaveLength(1);
expect(chunks[0]).toHaveLength(1);
assert.ok(chunks[0]);
expect(chunks[0][0]?.to).toHaveLength(100);
});

test('one message with 101 recipients', () => {
const messages = [{ to: new Array(101).fill('?') }];
const chunks = client.chunkPushNotifications(messages);
expect(chunks.length).toBe(2);
expect(chunks[0].length).toBe(1);
expect(chunks[1].length).toBe(1);
expect(chunks).toHaveLength(2);
expect(chunks[0]).toHaveLength(1);
expect(chunks[1]).toHaveLength(1);
const totalMessageCount = countAndValidateMessages(chunks);
expect(totalMessageCount).toBe(101);
});

test('one message with 99 recipients and two additional messages', () => {
const messages = [{ to: new Array(99).fill('?') }, ...new Array(2).fill({ to: '?' })];
const chunks = client.chunkPushNotifications(messages);
expect(chunks.length).toBe(2);
expect(chunks[0].length).toBe(2);
expect(chunks[1].length).toBe(1);
expect(chunks).toHaveLength(2);
expect(chunks[0]).toHaveLength(2);
expect(chunks[1]).toHaveLength(1);
const totalMessageCount = countAndValidateMessages(chunks);
expect(totalMessageCount).toBe(99 + 2);
});

test('one message with 100 recipients and two additional messages', () => {
const messages = [{ to: new Array(100).fill('?') }, ...new Array(2).fill({ to: '?' })];
const chunks = client.chunkPushNotifications(messages);
expect(chunks.length).toBe(2);
expect(chunks[0].length).toBe(1);
expect(chunks[1].length).toBe(2);
expect(chunks).toHaveLength(2);
expect(chunks[0]).toHaveLength(1);
expect(chunks[1]).toHaveLength(2);
const totalMessageCount = countAndValidateMessages(chunks);
expect(totalMessageCount).toBe(100 + 2);
});

test('99 messages and one additional message with with two recipients', () => {
const messages = [...new Array(99).fill({ to: '?' }), { to: new Array(2).fill('?') }];
const chunks = client.chunkPushNotifications(messages);
expect(chunks.length).toBe(2);
expect(chunks[0].length).toBe(100);
expect(chunks[1].length).toBe(1);
expect(chunks).toHaveLength(2);
expect(chunks[0]).toHaveLength(100);
expect(chunks[1]).toHaveLength(1);
const totalMessageCount = countAndValidateMessages(chunks);
expect(totalMessageCount).toBe(99 + 2);
});

test('no message', () => {
const messages: ExpoPushMessage[] = [];
const chunks = client.chunkPushNotifications(messages);
expect(chunks.length).toBe(0);
expect(chunks).toHaveLength(0);
});

test('one message with no recipient', () => {
const messages = [{ to: [] }];
const chunks = client.chunkPushNotifications(messages);
expect(chunks.length).toBe(0);
expect(chunks).toHaveLength(0);
});

test('two messages and one additional message with no recipient', () => {
const messages = [...new Array(2).fill({ to: '?' }), { to: [] }];
const chunks = client.chunkPushNotifications(messages);
expect(chunks.length).toBe(1);
expect(chunks).toHaveLength(1);
// The message with no recipient should be removed.
expect(chunks[0].length).toBe(2);
expect(chunks[0]).toHaveLength(2);
const totalMessageCount = countAndValidateMessages(chunks);
expect(totalMessageCount).toBe(2);
});
Expand Down
9 changes: 9 additions & 0 deletions tsconfig.build.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
{
"extends": "./tsconfig.json",
"compilerOptions": {
"noEmit": false,
"outDir": "./build",
"rootDir": "./src"
},
"exclude": ["node_modules", "**/__mocks__", "**/__tests__", "build"]
}
14 changes: 4 additions & 10 deletions tsconfig.json
Original file line number Diff line number Diff line change
@@ -1,15 +1,9 @@
{
"extends": ["@tsconfig/strictest/tsconfig.json", "@tsconfig/node-lts/tsconfig.json"],
"compilerOptions": {
"target": "es2015",
"lib": ["es2015"],
"module": "commonjs",
"sourceMap": true,
"declaration": true,
"types": ["jest", "node"],
"esModuleInterop": true,
"outDir": "./build",
"rootDir": "./src",
"strict": true
"composite": true,
"noEmit": true
},
"exclude": ["node_modules", "**/__mocks__", "**/__tests__", "build"]
"exclude": ["build"]
}
Loading

0 comments on commit 5fb64fa

Please sign in to comment.