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

style: enable strict and type-checked lints on eslint #228

Merged
merged 5 commits into from
Jun 26, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 26 additions & 2 deletions .eslintrc.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,14 @@ module.exports = {
parser: '@typescript-eslint/parser',
parserOptions: {
sourceType: 'module',
project: ['./tsconfig.json', './tsconfig.test.json'],
},
plugins: ['@typescript-eslint', 'security', 'jsdoc', 'import', 'simple-import-sort'],
extends: [
'eslint:recommended',
'plugin:@typescript-eslint/recommended',
'plugin:@typescript-eslint/recommended-type-checked',
'plugin:@typescript-eslint/strict-type-checked',
'plugin:@typescript-eslint/stylistic-type-checked',
'plugin:security/recommended-legacy',
'plugin:import/recommended',
],
Expand All @@ -35,6 +38,7 @@ module.exports = {
// TypeScript already enforces these rules better than any eslint setup can
'no-undef': 'off',
'no-dupe-class-members': 'off',
'require-await': 'off',
// see:
// https://github.com/ably/spaces/issues/76
// https://github.com/microsoft/TypeScript/issues/16577#issuecomment-703190339
Expand All @@ -47,8 +51,28 @@ module.exports = {
],
},
},
{
files: ['test/**/*.{ts,tsx}'],
rules: {
'@typescript-eslint/unbound-method': 'off',
'@typescript-eslint/no-empty-function': 'off',
},
},
],
ignorePatterns: [
'.eslintrc.cjs',
'dist',
'node_modules',
'ably-common',
'typedoc',
'src/utils',
'test/utils',
'scripts/cdn_deploy.js',
'vitest.config.ts',
'vite.config.ts',
'test/helper/testSetup.ts',
'__mocks__',
],
ignorePatterns: ['dist', 'node_modules', 'ably-common', 'typedoc', 'src/utils', 'scripts/cdn_deploy.js'],
settings: {
jsdoc: {
tagNamePreference: {
Expand Down
15 changes: 9 additions & 6 deletions __mocks__/ably/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,10 @@ import * as Ably from 'ably';

const MOCK_CLIENT_ID = 'MOCK_CLIENT_ID';

const mockPromisify = <T>(expectedReturnValue): Promise<T> => new Promise((resolve) => resolve(expectedReturnValue));
const mockPromisify = <T>(expectedReturnValue): Promise<T> =>
new Promise((resolve) => {
resolve(expectedReturnValue);
});
const methodReturningVoidPromise = () => mockPromisify<void>((() => {})());

function createMockPresence() {
Expand All @@ -18,8 +21,8 @@ function createMockPresence() {
fn();
},
},
subscribe: () => {},
unsubscribe: () => {},
subscribe: methodReturningVoidPromise,
unsubscribe: methodReturningVoidPromise,
};
}

Expand All @@ -37,8 +40,8 @@ function createMockChannel() {
attach: methodReturningVoidPromise,
detach: methodReturningVoidPromise,
presence: createMockPresence(),
subscribe: () => {},
unsubscribe: () => {},
subscribe: methodReturningVoidPromise,
unsubscribe: methodReturningVoidPromise,
on: () => {},
off: () => {},
publish: () => {},
Expand Down Expand Up @@ -79,7 +82,7 @@ class MockRealtime {
state: 'connected',
};

this['options'] = {};
this.options = {};
}
}

Expand Down
9 changes: 4 additions & 5 deletions src/ChatApi.ts
Original file line number Diff line number Diff line change
Expand Up @@ -70,19 +70,18 @@ export class ChatApi {
method: 'POST' | 'GET' | ' PUT' | 'DELETE' | 'PATCH',
body?: REQ,
): Promise<RES> {
const response = await this.realtime.request(method, url, 1.1, {}, body);
const response = await this.realtime.request<RES>(method, url, 1.1, {}, body);
if (!response.success) {
this._logger.error('ChatApi.makeAuthorisedRequest(); failed to make request', {
url,
statusCode: response.statusCode,
errorCode: response.errorCode,
errorMessage: response.errorMessage,
});
throw new Ably.ErrorInfo(response.errorMessage, response.errorCode, response.statusCode);
throw new Ably.ErrorInfo(response.errorMessage, response.errorCode, response.statusCode) as unknown as Error;
}

const [result] = response.items;
return result as RES;
return response.items[0] as RES;
}

private async makeAuthorisedPaginatedRequest<RES, REQ = undefined>(
Expand All @@ -98,7 +97,7 @@ export class ChatApi {
errorCode: response.errorCode,
errorMessage: response.errorMessage,
});
throw new Ably.ErrorInfo(response.errorMessage, response.errorCode, response.statusCode);
throw new Ably.ErrorInfo(response.errorMessage, response.errorCode, response.statusCode) as unknown as Error;
}
return response;
}
Expand Down
8 changes: 6 additions & 2 deletions src/Message.ts
Original file line number Diff line number Diff line change
Expand Up @@ -106,9 +106,13 @@ export class DefaultMessage implements Message {
*/
private static timeserialCompare(first: Message, second: Message): number {
const firstTimeserial =
(first as DefaultMessage)._calculatedTimeserial ?? DefaultMessage.calculateTimeserial(first.timeserial);
first instanceof DefaultMessage
? first._calculatedTimeserial
: DefaultMessage.calculateTimeserial(first.timeserial);
const secondTimeserial =
(second as DefaultMessage)._calculatedTimeserial ?? DefaultMessage.calculateTimeserial(second.timeserial);
second instanceof DefaultMessage
? second._calculatedTimeserial
: DefaultMessage.calculateTimeserial(second.timeserial);

// Compare the timestamp
const timestampDiff = firstTimeserial.timestamp - secondTimeserial.timestamp;
Expand Down
42 changes: 30 additions & 12 deletions src/Messages.ts
Original file line number Diff line number Diff line change
Expand Up @@ -192,7 +192,7 @@ export class DefaultMessages extends EventEmitter<MessageEventsMap> implements M
if (!hasListeners) {
this._logger.debug('Messages.subscribe(); subscribing internal listener');
this._internalListener = this.processEvent.bind(this);
return this._managedChannel.subscribe([MessageEvents.created], this._internalListener!);
return this._managedChannel.subscribe([MessageEvents.created], this._internalListener);
}

return this._managedChannel.channel.attach();
Expand All @@ -209,7 +209,11 @@ export class DefaultMessages extends EventEmitter<MessageEventsMap> implements M
}

this._logger.debug('Messages.unsubscribe(); unsubscribing internal listener');
return this._managedChannel.unsubscribe(this._internalListener!);
if (this._internalListener) {
return this._managedChannel.unsubscribe(this._internalListener);
}

return Promise.resolve();
}

private processEvent(channelEventMessage: Ably.InboundMessage) {
Expand Down Expand Up @@ -238,40 +242,54 @@ export class DefaultMessages extends EventEmitter<MessageEventsMap> implements M
* Validate the realtime message and convert it to a chat message.
*/
private parseNewMessage(channelEventMessage: Ably.InboundMessage): Message | undefined {
const { data, clientId, timestamp, extras } = channelEventMessage;
Copy link
Contributor

@vladvelici vladvelici Jun 26, 2024

Choose a reason for hiding this comment

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

Is this too evil?

Edit to explain: what's the rule that this was breaking? I know it might not be this line, but just curious what can't we do? Is it the type of extras being not specific enough?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's because data and extras are any types, so it's trying to say that you shouldn't set any to a typed variable because that may go unnoticed and leak into your codebase:

https://typescript-eslint.io/rules/no-unsafe-assignment/

interface MessagePayload {
data?: {
content?: string;
};
clientId?: string;
timestamp?: number;
extras?: {
timeserial?: string;
};
}
const messageCreatedMessage = channelEventMessage as MessagePayload;

if (!data) {
if (!messageCreatedMessage.data) {
this._logger.error(`received incoming message without data`, channelEventMessage);
return;
}

if (!clientId) {
if (!messageCreatedMessage.clientId) {
this._logger.error(`received incoming message without clientId`, channelEventMessage);
return;
}

if (!timestamp) {
if (!messageCreatedMessage.timestamp) {
this._logger.error(`received incoming message without timestamp`, channelEventMessage);
return;
}

const { content } = data;
if (!content) {
if (messageCreatedMessage.data.content === undefined) {
this._logger.error(`received incoming message without content`, channelEventMessage);
return;
}

if (!extras) {
if (!messageCreatedMessage.extras) {
this._logger.error(`received incoming message without extras`, channelEventMessage);
return;
}

const { timeserial } = extras;
if (!timeserial) {
if (!messageCreatedMessage.extras.timeserial) {
this._logger.error(`received incoming message without timeserial`, channelEventMessage);
return;
}

return new DefaultMessage(timeserial, clientId, this._roomId, content, new Date(timestamp));
return new DefaultMessage(
messageCreatedMessage.extras.timeserial,
messageCreatedMessage.clientId,
this._roomId,
messageCreatedMessage.data.content,
new Date(messageCreatedMessage.timestamp),
);
}
}
14 changes: 5 additions & 9 deletions src/Occupancy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ export interface Occupancy {
/**
* Represents the occupancy of a chat room.
*/
export type OccupancyEvent = {
export interface OccupancyEvent {
/**
* The number of connections to the chat room.
*/
Expand All @@ -56,7 +56,7 @@ export type OccupancyEvent = {
* The number of presence members in the chat room - members who have entered presence.
*/
presenceMembers: number;
};
}

/**
* A listener that is called when the occupancy of a chat room changes.
Expand Down Expand Up @@ -99,7 +99,7 @@ export class DefaultOccupancy extends EventEmitter<OccupancyEventsMap> implement
this._logger.debug('Occupancy.subscribe(); adding internal listener');
this._internalListener = this.internalOccupancyListener.bind(this);
return this._managedChannel
.subscribe(['[meta]occupancy'], this._internalListener as Ably.messageCallback<Ably.InboundMessage>)
.subscribe(['[meta]occupancy'], this._internalListener)
.then(async (stateChange: Ably.ChannelStateChange | null) => {
await this._managedChannel.channel.setOptions({ params: { occupancy: 'metrics' } });
return stateChange;
Expand All @@ -119,9 +119,7 @@ export class DefaultOccupancy extends EventEmitter<OccupancyEventsMap> implement
this._logger.debug('Occupancy.unsubscribe(); removing internal listener');
return this._managedChannel.channel
.setOptions({})
.then(() =>
this._managedChannel.unsubscribe(this._internalListener as Ably.messageCallback<Ably.InboundMessage>),
)
.then(() => (this._internalListener ? this._managedChannel.unsubscribe(this._internalListener) : null))
.then(() => {
this._internalListener = undefined;
});
Expand Down Expand Up @@ -150,9 +148,7 @@ export class DefaultOccupancy extends EventEmitter<OccupancyEventsMap> implement
* occupancy events for the public API.
*/
private internalOccupancyListener(message: Ably.InboundMessage): void {
const {
data: { metrics },
} = message;
const { metrics } = message.data as { metrics?: { connections?: number; presenceMembers?: number } };

if (metrics === undefined) {
this._logger.error('invalid occupancy event received; metrics is missing', message);
Expand Down
37 changes: 25 additions & 12 deletions src/Presence.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,7 @@ interface PresenceEventsMap {
/**
* Type for PresenceData
*/
export type PresenceData = {
[key: string]: unknown;
};
export type PresenceData = Record<string, unknown>;

/**
* Type for AblyPresenceData
Expand Down Expand Up @@ -209,13 +207,27 @@ export class DefaultPresence extends EventEmitter<PresenceEventsMap> implements
async get(params?: Ably.RealtimePresenceParams): Promise<PresenceMember[]> {
this._logger.trace('Presence.get()', { params });
const userOnPresence = await this.subscriptionManager.channel.presence.get(params);
const userDataToReturn = (data: string) => {
try {
const parsedData = JSON.parse(data) as AblyPresenceData;
if (!parsedData.userCustomData) {
return undefined;
}

return parsedData.userCustomData;
} catch (error) {
this._logger.error('Presence.get(); error parsing user data', { error });
return undefined;
}
};

// ably-js never emits the 'absent' event, so we can safely ignore it here.
return userOnPresence.map((user) => ({
clientId: user.clientId,
action: user.action as PresenceEvents,
data: user.data ? (JSON.parse(user.data).userCustomData as PresenceData) : undefined,
data: userDataToReturn(user.data as string),
timestamp: user.timestamp,
// eslint-disable-next-line @typescript-eslint/no-unsafe-assignment
extras: user.extras,
id: user.id,
}));
Expand Down Expand Up @@ -286,7 +298,7 @@ export class DefaultPresence extends EventEmitter<PresenceEventsMap> implements
this._logger.trace('Presence.subscribe(); listenerOrEvents', { listenerOrEvents });
if (!listenerOrEvents && !listener) {
this._logger.error('could not subscribe to presence; invalid arguments');
throw new Ably.ErrorInfo('could not subscribe listener: invalid arguments', 40000, 400);
throw new Ably.ErrorInfo('could not subscribe listener: invalid arguments', 40000, 400) as unknown as Error;
}
const hasListeners = this.hasListeners();
if (!listener) {
Expand All @@ -298,7 +310,9 @@ export class DefaultPresence extends EventEmitter<PresenceEventsMap> implements
this._logger.debug('Presence.subscribe(); adding internal listener');
return this.subscriptionManager.presenceSubscribe(this.subscribeToEvents);
}
return this.subscriptionManager.channel.attach().then(() => {});
return this.subscriptionManager.channel.attach().then(() => {
return Promise.resolve();
});
}

/**
Expand All @@ -320,7 +334,7 @@ export class DefaultPresence extends EventEmitter<PresenceEventsMap> implements
this._logger.trace('Presence.unsubscribe(); listenerOrEvents', { listenerOrEvents });
if (!listenerOrEvents && !listener) {
this._logger.error('could not unsubscribe from presence; invalid arguments');
throw new Ably.ErrorInfo('could not unsubscribe listener: invalid arguments', 40000, 400);
throw new Ably.ErrorInfo('could not unsubscribe listener: invalid arguments', 40000, 400) as unknown as Error;
}
if (!listener) {
this.off(listenerOrEvents);
Expand All @@ -342,11 +356,10 @@ export class DefaultPresence extends EventEmitter<PresenceEventsMap> implements
*/
subscribeToEvents = (member: Ably.PresenceMessage) => {
try {
const parsedData = JSON.parse(member.data);

const parsedData = JSON.parse(member.data as string) as AblyPresenceData;
// ably-js never emits the 'absent' event, so we can safely ignore it here.
this.emit(PresenceEvents[member.action as PresenceEvents], {
action: PresenceEvents[member.action as PresenceEvents],
this.emit(member.action as PresenceEvents, {
action: member.action as PresenceEvents,
clientId: member.clientId,
timestamp: member.timestamp,
data: parsedData.userCustomData,
Expand All @@ -358,7 +371,7 @@ export class DefaultPresence extends EventEmitter<PresenceEventsMap> implements
50000,
500,
(error as Error).message,
);
) as unknown as Error;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure what's going on here, but it's odd to have to as unknown as Error all over the place. Can we make this not required or add ticket to make it so later?

Maybe a potential fix is to make a constructor function that does the casting?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is because ErrorInfo doesn't extend Error in the ably-js types :(

I've already merged the fix in ably/ably-js#1805 so we can remove this in the next version of ably-js

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice!

}
};
}
Loading
Loading