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

[No QA] [TS migration] Migrate 'Pusher' lib to TypeScript #28116

Merged
merged 14 commits into from
Oct 13, 2023
Merged
kubabutkiewicz marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -11,4 +11,4 @@ export default {
MULTIPLE_EVENT_TYPE: {
ONYX_API_UPDATE: 'onyxApiUpdate',
},
};
} as const;
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
* We use the pusher-js/react-native module to support pusher on native environments.
* @see: https://github.com/pusher/pusher-js
*/
import Pusher from 'pusher-js/react-native';
import PusherNative from 'pusher-js/react-native';
import Pusher from './types';

export default Pusher;
export default PusherNative satisfies Pusher;
Copy link
Contributor

Choose a reason for hiding this comment

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

From TS guidelines:

Do not use satisfies operator for platform-specific implementations, always define shared types that complies with all variants.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm , do you have an idea how to type it then? in this case platform specific files exports classes not functions , I dont see better solution for that 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

How about this:

import type Pusher from './types.ts';
import PusherImplementation from 'pusher-js/with-encryption';

const PusherWeb: Pusher = PusherImplementation;

export default PusherWeb;

Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
* We use the standard pusher-js module to support pusher on web environments.
* @see: https://github.com/pusher/pusher-js
*/
import Pusher from 'pusher-js/with-encryption';
import PusherWeb from 'pusher-js/with-encryption';
import Pusher from './types';

export default Pusher;
export default PusherWeb satisfies Pusher;
10 changes: 10 additions & 0 deletions src/libs/Pusher/library/types.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
import PusherClass from 'pusher-js/with-encryption';
import {LiteralUnion} from 'type-fest';

type Pusher = typeof PusherClass;

type SocketEventName = LiteralUnion<'error' | 'connected' | 'disconnected' | 'state_change', string>;

export default Pusher;

export type {SocketEventName};
156 changes: 68 additions & 88 deletions src/libs/Pusher/pusher.js → src/libs/Pusher/pusher.ts
Original file line number Diff line number Diff line change
@@ -1,9 +1,42 @@
import Onyx from 'react-native-onyx';
import _ from 'underscore';
import {Channel, ChannelAuthorizerGenerator, Options} from 'pusher-js/with-encryption';
import isObject from 'lodash/isObject';
import {LiteralUnion} from 'type-fest';
import ONYXKEYS from '../../ONYXKEYS';
import Pusher from './library';
import TYPE from './EventType';
import Log from '../Log';
import DeepValueOf from '../../types/utils/DeepValueOf';
import {SocketEventName} from './library/types';

type States = {
previous: string;
current: string;
};

type Args = {
appKey: string;
cluster: string;
authEndpoint: string;
};

type ChunkedDataEvents = {chunks: unknown[]; receivedFinal: boolean};

type EventData = {id?: string; chunk?: unknown; final?: boolean; index: number};

type SocketEventCallback<T> = (eventName: SocketEventName, data?: T) => void;

type PusherWithAuthParams = InstanceType<typeof Pusher> & {
config: {
auth?: {
params?: unknown;
};
};
};

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
type PusherEventName = LiteralUnion<DeepValueOf<typeof TYPE>, string>;

We'll need this one for the next functions.

type PusherEventName = LiteralUnion<DeepValueOf<typeof TYPE>, string>;

type PusherSubscribtionErrorData = {type?: string; error?: string; status?: string};

let shouldForceOffline = false;
Onyx.connect({
Expand All @@ -16,33 +49,23 @@ Onyx.connect({
},
});

let socket;
let socket: PusherWithAuthParams | null;
let pusherSocketID = '';
const socketEventCallbacks = [];
let customAuthorizer;
const socketEventCallbacks: Array<SocketEventCallback<unknown>> = [];
let customAuthorizer: ChannelAuthorizerGenerator;

/**
* Trigger each of the socket event callbacks with the event information
*
* @param {String} eventName
* @param {*} data
*/
function callSocketEventCallbacks(eventName, data) {
_.each(socketEventCallbacks, (cb) => cb(eventName, data));
function callSocketEventCallbacks<T>(eventName: SocketEventName, data?: T) {
socketEventCallbacks.forEach((cb) => cb(eventName, data));
}

/**
* Initialize our pusher lib
*
* @param {Object} args
* @param {String} args.appKey
* @param {String} args.cluster
* @param {String} args.authEndpoint
Comment on lines -37 to -40
Copy link
Contributor

Choose a reason for hiding this comment

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

NAB: Sorry if I'm not up to date with the latest guidelines, but shouldn't we add a type for args if we already defined the types in JSDoc?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, for typescript migrations we are removing all JSDocs types we are leaving only params descriptions

* @param {Object} [params]
* @public
* @returns {Promise} resolves when Pusher has connected
* @returns resolves when Pusher has connected
*/
function init(args, params) {
function init(args: Args, params?: unknown): Promise<void> {
return new Promise((resolve) => {
if (socket) {
return resolve();
Expand All @@ -55,7 +78,7 @@ function init(args, params) {
// }
// };

const options = {
const options: Options = {
cluster: args.cluster,
authEndpoint: args.authEndpoint,
};
Expand All @@ -65,7 +88,6 @@ function init(args, params) {
}

socket = new Pusher(args.appKey, options);

// If we want to pass params in our requests to api.php we'll need to add it to socket.config.auth.params
// as per the documentation
// (https://pusher.com/docs/channels/using_channels/connection#channels-options-parameter).
Expand All @@ -77,34 +99,30 @@ function init(args, params) {
}

// Listen for connection errors and log them
socket.connection.bind('error', (error) => {
socket?.connection.bind('error', (error: unknown) => {
callSocketEventCallbacks('error', error);
});

socket.connection.bind('connected', () => {
pusherSocketID = socket.connection.socket_id;
socket?.connection.bind('connected', () => {
pusherSocketID = socket?.connection.socket_id ?? '';
callSocketEventCallbacks('connected');
resolve();
});

socket.connection.bind('disconnected', () => {
socket?.connection.bind('disconnected', () => {
callSocketEventCallbacks('disconnected');
});

socket.connection.bind('state_change', (states) => {
socket?.connection.bind('state_change', (states: States) => {
callSocketEventCallbacks('state_change', states);
});
});
}

/**
* Returns a Pusher channel for a channel name
*
* @param {String} channelName
*
* @returns {Channel}
*/
function getChannel(channelName) {
function getChannel(channelName: string): Channel | undefined {
if (!socket) {
return;
}
Expand All @@ -114,27 +132,22 @@ function getChannel(channelName) {

/**
* Binds an event callback to a channel + eventName
* @param {Pusher.Channel} channel
* @param {String} eventName
* @param {Function} [eventCallback]
*
* @private
*/
function bindEventToChannel(channel, eventName, eventCallback = () => {}) {
function bindEventToChannel(channel: Channel | undefined, eventName: PusherEventName, eventCallback: (data: unknown) => void = () => {}) {
if (!eventName) {
return;
}

const chunkedDataEvents = {};
const callback = (eventData) => {
const chunkedDataEvents: Record<string, ChunkedDataEvents> = {};
const callback = (eventData: string | Record<string, unknown>) => {
if (shouldForceOffline) {
Log.info('[Pusher] Ignoring a Push event because shouldForceOffline = true');
return;
}

let data;
let data: EventData;
try {
data = _.isObject(eventData) ? eventData : JSON.parse(eventData);
data = isObject(eventData) ? eventData : JSON.parse(eventData);
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we migrating from lodash in a scope of TS migration?

Suggested change
data = isObject(eventData) ? eventData : JSON.parse(eventData);
data = typeof eventData === 'object' ? eventData : JSON.parse(eventData);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Lodash usage is allowed, we are migrating from underscore. And I think usage of lodash here is more safe, because if data will be Array type then typeof data will still be object for example same if data would be a Date

} catch (err) {
Log.alert('[Pusher] Unable to parse single JSON event data from Pusher', {error: err, eventData});
return;
Expand Down Expand Up @@ -164,7 +177,7 @@ function bindEventToChannel(channel, eventName, eventCallback = () => {}) {

// Only call the event callback if we've received the last packet and we don't have any holes in the complete
// packet.
if (chunkedEvent.receivedFinal && chunkedEvent.chunks.length === _.keys(chunkedEvent.chunks).length) {
if (chunkedEvent.receivedFinal && chunkedEvent.chunks.length === Object.keys(chunkedEvent.chunks).length) {
try {
eventCallback(JSON.parse(chunkedEvent.chunks.join('')));
} catch (err) {
Expand All @@ -181,22 +194,14 @@ function bindEventToChannel(channel, eventName, eventCallback = () => {}) {
}
};

channel.bind(eventName, callback);
channel?.bind(eventName, callback);
}

/**
* Subscribe to a channel and an event
*
* @param {String} channelName
* @param {String} eventName
* @param {Function} [eventCallback]
* @param {Function} [onResubscribe] Callback to be called when reconnection happen
*
* @return {Promise}
*
* @public
* @param [onResubscribe] Callback to be called when reconnection happen
*/
function subscribe(channelName, eventName, eventCallback = () => {}, onResubscribe = () => {}) {
function subscribe(channelName: string, eventName: PusherEventName, eventCallback = () => {}, onResubscribe = () => {}): Promise<void> {
return new Promise((resolve, reject) => {
// We cannot call subscribe() before init(). Prevent any attempt to do this on dev.
if (!socket) {
Expand Down Expand Up @@ -226,7 +231,7 @@ function subscribe(channelName, eventName, eventCallback = () => {}, onResubscri
onResubscribe();
});

channel.bind('pusher:subscription_error', (data = {}) => {
channel.bind('pusher:subscription_error', (data: PusherSubscribtionErrorData = {}) => {
const {type, error, status} = data;
Log.hmmm('[Pusher] Issue authenticating with Pusher during subscribe attempt.', {
channelName,
Expand All @@ -245,12 +250,8 @@ function subscribe(channelName, eventName, eventCallback = () => {}, onResubscri

/**
* Unsubscribe from a channel and optionally a specific event
*
* @param {String} channelName
* @param {String} [eventName]
* @public
*/
function unsubscribe(channelName, eventName = '') {
function unsubscribe(channelName: string, eventName: PusherEventName = '') {
const channel = getChannel(channelName);

if (!channel) {
Expand All @@ -269,18 +270,14 @@ function unsubscribe(channelName, eventName = '') {
Log.info('[Pusher] Unsubscribing from channel', false, {channelName});

channel.unbind();
socket.unsubscribe(channelName);
socket?.unsubscribe(channelName);
}
}

/**
* Are we already in the process of subscribing to this channel?
*
* @param {String} channelName
*
* @returns {Boolean}
*/
function isAlreadySubscribing(channelName) {
function isAlreadySubscribing(channelName: string): boolean {
if (!socket) {
return false;
}
Expand All @@ -291,12 +288,8 @@ function isAlreadySubscribing(channelName) {

/**
* Are we already subscribed to this channel?
*
* @param {String} channelName
*
* @returns {Boolean}
*/
function isSubscribed(channelName) {
function isSubscribed(channelName: string): boolean {
if (!socket) {
return false;
}
Expand All @@ -307,39 +300,31 @@ function isSubscribed(channelName) {

/**
* Sends an event over a specific event/channel in pusher.
*
* @param {String} channelName
* @param {String} eventName
* @param {Object} payload
*/
function sendEvent(channelName, eventName, payload) {
function sendEvent<T>(channelName: string, eventName: PusherEventName, payload: T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the purpose of generic type here? We need to better type payload or at least limit the generic type so it extends Record.

// Check to see if we are subscribed to this channel before sending the event. Sending client events over channels
// we are not subscribed too will throw errors and cause reconnection attempts. Subscriptions are not instant and
// can happen later than we expect.
if (!isSubscribed(channelName)) {
return;
}

socket.send_event(eventName, payload, channelName);
socket?.send_event(eventName, payload, channelName);
}

/**
* Register a method that will be triggered when a socket event happens (like disconnecting)
*
* @param {Function} cb
*/
function registerSocketEventCallback(cb) {
socketEventCallbacks.push(cb);
function registerSocketEventCallback<T>(cb: SocketEventCallback<T>) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here, why a generic type?

socketEventCallbacks.push(cb as SocketEventCallback<unknown>);
}

/**
* A custom authorizer allows us to take a more fine-grained approach to
* authenticating Pusher. e.g. we can handle failed attempts to authorize
* with an expired authToken and retry the attempt.
*
* @param {Function} authorizer
*/
function registerCustomAuthorizer(authorizer) {
function registerCustomAuthorizer(authorizer: ChannelAuthorizerGenerator) {
customAuthorizer = authorizer;
}

Expand Down Expand Up @@ -371,18 +356,13 @@ function reconnect() {
socket.connect();
}

/**
* @returns {String}
*/
function getPusherSocketID() {
function getPusherSocketID(): string {
return pusherSocketID;
}

if (window) {
/**
* Pusher socket for debugging purposes
*
* @returns {Function}
*/
window.getPusherInstance = () => socket;
}
Expand Down
Loading
Loading