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

[TS migration] Migrate 'Device' lib to TypeScript #27709

Merged
merged 20 commits into from
Nov 9, 2023
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
12 changes: 6 additions & 6 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@
"date-fns-tz": "^2.0.0",
"dom-serializer": "^0.2.2",
"domhandler": "^4.3.0",
"expensify-common": "git+ssh://git@github.com/Expensify/expensify-common.git#a055da6a339383da825f353d5c0da72a26285007",
"expensify-common": "git+ssh://git@github.com/Expensify/expensify-common.git#886f90cbd5e83218fdfd7784d8356c308ef05791",
"fbjs": "^3.0.2",
"htmlparser2": "^7.2.0",
"idb-keyval": "^6.2.1",
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import Str from 'expensify-common/lib/str';
import DeviceInfo from 'react-native-device-info';
import GenerateDeviceID from './types';

const deviceID = DeviceInfo.getDeviceId();
const uniqueID = Str.guid(deviceID);
Expand All @@ -22,11 +23,7 @@ const uniqueID = Str.guid(deviceID);
*
* Furthermore, the deviceID prefix is not unique to a specific device, but is likely to change from one type of device to another.
* Including this prefix will tell us with a reasonable degree of confidence if the user just uninstalled and reinstalled the app, or if they got a new device.
*
* @returns {Promise<String>}
*/
function generateDeviceID() {
return Promise.resolve(uniqueID);
}
const generateDeviceID: GenerateDeviceID = () => Promise.resolve(uniqueID);

export default generateDeviceID;
Original file line number Diff line number Diff line change
@@ -1,12 +1,10 @@
import ELECTRON_EVENTS from '../../../../../desktop/ELECTRON_EVENTS';
import GenerateDeviceID from './types';

/**
* Get the unique ID of the current device. This should remain the same even if the user uninstalls and reinstalls the app.
*
* @returns {Promise<String>}
*/
function generateDeviceID() {
return window.electron.invoke(ELECTRON_EVENTS.REQUEST_DEVICE_ID);
}

const generateDeviceID: GenerateDeviceID = () => window.electron.invoke(ELECTRON_EVENTS.REQUEST_DEVICE_ID) as Promise<string>;

export default generateDeviceID;
Original file line number Diff line number Diff line change
@@ -1,14 +1,11 @@
import DeviceInfo from 'react-native-device-info';
import GenerateDeviceID from './types';

const deviceID = DeviceInfo.getDeviceId();

/**
* Get the unique ID of the current device. This should remain the same even if the user uninstalls and reinstalls the app.
*
* @returns {Promise<String>}
*/
function generateDeviceID() {
return DeviceInfo.getUniqueId().then((uniqueID) => `${deviceID}_${uniqueID}`);
}
const generateDeviceID: GenerateDeviceID = () => DeviceInfo.getUniqueId().then((uniqueID: string) => `${deviceID}_${uniqueID}`);

export default generateDeviceID;
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import Str from 'expensify-common/lib/str';
import GenerateDeviceID from './types';

const uniqueID = Str.guid();

Expand All @@ -13,11 +14,8 @@ const uniqueID = Str.guid();
*
* While this isn't perfect, it's just as good as any other obvious web solution, such as this one https://developer.mozilla.org/en-US/docs/Web/API/MediaDeviceInfo/deviceId
* which is also different/reset under the same circumstances
*
* @returns {Promise<String>}
*/
function generateDeviceID() {
return Promise.resolve(uniqueID);
}

const generateDeviceID: GenerateDeviceID = () => Promise.resolve(uniqueID);

export default generateDeviceID;
3 changes: 3 additions & 0 deletions src/libs/actions/Device/generateDeviceID/types.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
type GenerateDeviceID = () => Promise<string>;

export default GenerateDeviceID;
8 changes: 0 additions & 8 deletions src/libs/actions/Device/getDeviceInfo/getBaseInfo.js

This file was deleted.

9 changes: 9 additions & 0 deletions src/libs/actions/Device/getDeviceInfo/getBaseInfo.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
import packageConfig from '../../../../../package.json';
import {GetBaseInfo} from './types';

const getBaseInfo: GetBaseInfo = () => ({
appVersion: packageConfig.version,
timestamp: new Date().toISOString().slice(0, 19),
});

export default getBaseInfo;
Original file line number Diff line number Diff line change
@@ -1,17 +1,13 @@
import Str from 'expensify-common/lib/str';
import RNDeviceInfo from 'react-native-device-info';
import GetOSAndName from './types';
import {GetOSAndName} from './types';

const getOSAndName: GetOSAndName = () => {
const deviceName = RNDeviceInfo.getDeviceNameSync();
const prettyName = `${Str.UCFirst(RNDeviceInfo.getManufacturerSync() || '')} ${deviceName}`;
return {
// Parameter names are predefined and we don't choose it here
// eslint-disable-next-line @typescript-eslint/naming-convention
device_name: RNDeviceInfo.isEmulatorSync() ? `Emulator - ${prettyName}` : prettyName,
// Parameter names are predefined and we don't choose it here
// eslint-disable-next-line @typescript-eslint/naming-convention
os_version: RNDeviceInfo.getSystemVersion(),
deviceName: RNDeviceInfo.isEmulatorSync() ? `Emulator - ${prettyName}` : prettyName,
osVersion: RNDeviceInfo.getSystemVersion(),
};
};

Expand Down
12 changes: 2 additions & 10 deletions src/libs/actions/Device/getDeviceInfo/getOSAndName/index.ts
Original file line number Diff line number Diff line change
@@ -1,12 +1,4 @@
import {getOSAndName as libGetOSAndName} from 'expensify-common/lib/Device';
import GetOSAndName from './types';
// Don't import this file with '* as Device'. It's known to make VSCode IntelliSense crash.
import {getOSAndName} from 'expensify-common/lib/Device';

Copy link
Contributor

Choose a reason for hiding this comment

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

I am working on the PR, but when testing, I can see the command still passes the snake case params, which are coming from the expensify-common here. Is this intentional? I am a bit confused about this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, you are right @mountiny . I think we should leave these ones as they are, to avoid making additional changes to expensify-common. I checked the lib and there are a lot of exported properties there that don't follow naming conventions here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Alright, well then there is not much to do here. I am going to let the backend PR go out for review and update here when ready. I think the app_version is the only one

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah this is only for web so in native it will be sent differently

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, you are right @mountiny . I think we should leave these ones as they are, to avoid making additional changes to expensify-common. I checked the lib and there are a lot of exported properties there that don't follow naming conventions here.

I disagree, I think we should update them all to match our style guide. Is there a reason that can't be done? I understand it's more work, but we should follow our style guide

Copy link
Contributor

Choose a reason for hiding this comment

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

But netherless, in case we would do such changes we shouldn't block this PR because of it, as I understand expensify-common is used in several other projects, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

Surely we can do it, my point is: We defined and set these naming conventions for TS files in E/App, so it's the style guide of this project. expensify-common is a different library that doesn't even have a defined style guide I think, so change things in a library that is used across different Expensify projects just to satisfy E/App needs looks a bit odd to me 🤔

I'm pretty sure we use our public style guide in all our repos, at least our internal ones. But maybe our App style guide is different.

But netherless, in case we would do such changes we shouldn't block this PR because of it, as I understand expensify-common is used in several other projects, right?

If there's no updates needed in this code base after updating expensify-common, then we shouldn't block this PR on that update. Otherwise, I think we should update expensify-common first

Copy link
Contributor

Choose a reason for hiding this comment

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

I will update it tomorrow

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm I don't have access to this repo, but this naming conventions is an ESLint rule that we applied only to TS files in E/App.

If there's no updates needed in this code base after updating expensify-common, then we shouldn't block this PR on that update. Otherwise, I think we should update expensify-common first

@mountiny @cead22 We would have to change the params that are using snake case. So, do you want to proceed with expensify-common PR first and putting this on HOLD?

cc @kubabutkiewicz Since I will be OOO for the next two weeks.

Copy link
Contributor

Choose a reason for hiding this comment

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

I will make the expensify-common change

const getOSAndName: GetOSAndName = () => {
fvlvte marked this conversation as resolved.
Show resolved Hide resolved
// Parameter names are predefined and we don't choose it here
// eslint-disable-next-line @typescript-eslint/naming-convention
const {device_name, os_version} = libGetOSAndName();
// Parameter names are predefined and we don't choose it here
// eslint-disable-next-line @typescript-eslint/naming-convention
return {device_name, os_version};
};
export default getOSAndName;
11 changes: 7 additions & 4 deletions src/libs/actions/Device/getDeviceInfo/getOSAndName/types.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,7 @@
// Parameter names are predefined and we don't choose it here
// eslint-disable-next-line @typescript-eslint/naming-convention
type GetOSAndName = () => {device_name: string | undefined; os_version: string | undefined};
export default GetOSAndName;
type GetOSAndName = () => OSAndName;
type OSAndName = {
deviceName?: string;
osVersion?: string;
};

export type {GetOSAndName, OSAndName};
10 changes: 0 additions & 10 deletions src/libs/actions/Device/getDeviceInfo/index.android.js

This file was deleted.

11 changes: 11 additions & 0 deletions src/libs/actions/Device/getDeviceInfo/index.android.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
import getBaseInfo from './getBaseInfo';
import getOSAndName from './getOSAndName/index';
import {GetDeviceInfo} from './types';

const getDeviceInfo: GetDeviceInfo = () => ({
...getBaseInfo(),
...getOSAndName(),
os: 'Android',
});

export default getDeviceInfo;
10 changes: 0 additions & 10 deletions src/libs/actions/Device/getDeviceInfo/index.desktop.js

This file was deleted.

11 changes: 11 additions & 0 deletions src/libs/actions/Device/getDeviceInfo/index.desktop.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
import getBaseInfo from './getBaseInfo';
import getOSAndName from './getOSAndName/index';
import {GetDeviceInfo} from './types';

const getDeviceInfo: GetDeviceInfo = () => ({
...getBaseInfo(),
...getOSAndName(),
deviceName: 'Desktop',
});

export default getDeviceInfo;
10 changes: 0 additions & 10 deletions src/libs/actions/Device/getDeviceInfo/index.ios.js

This file was deleted.

11 changes: 11 additions & 0 deletions src/libs/actions/Device/getDeviceInfo/index.ios.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
import getBaseInfo from './getBaseInfo';
import getOSAndName from './getOSAndName/index';
import {GetDeviceInfo} from './types';

const getDeviceInfo: GetDeviceInfo = () => ({
...getBaseInfo(),
...getOSAndName(),
os: 'iOS',
});

export default getDeviceInfo;
23 changes: 0 additions & 23 deletions src/libs/actions/Device/getDeviceInfo/index.js

This file was deleted.

10 changes: 10 additions & 0 deletions src/libs/actions/Device/getDeviceInfo/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
import getBaseInfo from './getBaseInfo';
import getOSAndName from './getOSAndName/index';
import {GetDeviceInfo} from './types';

const getDeviceInfo: GetDeviceInfo = () => ({
...getBaseInfo(),
...getOSAndName(),
});

export default getDeviceInfo;
12 changes: 12 additions & 0 deletions src/libs/actions/Device/getDeviceInfo/types.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
import {OSAndName} from './getOSAndName/types';

type BaseInfo = {
appVersion: string;
timestamp: string;
};

type GetDeviceInfo = () => DeviceInfo;
type DeviceInfo = BaseInfo & OSAndName & {os?: string; deviceName?: string; deviceVersion?: string};
type GetBaseInfo = () => BaseInfo;

export type {GetDeviceInfo, DeviceInfo, GetBaseInfo, BaseInfo};
Original file line number Diff line number Diff line change
Expand Up @@ -4,23 +4,23 @@ import ONYXKEYS from '@src/ONYXKEYS';
import generateDeviceID from './generateDeviceID';
import getDeviceInfo from './getDeviceInfo';

let deviceID;
let deviceID: string | null = null;

/**
* @returns {Promise<String>}
* @returns - device ID string or null in case of failure
*/
function getDeviceID() {
function getDeviceID(): Promise<string | null> {
return new Promise((resolve) => {
if (deviceID) {
return resolve(deviceID);
}

const connectionID = Onyx.connect({
key: ONYXKEYS.DEVICE_ID,
callback: (ID) => {
callback: (id) => {
Onyx.disconnect(connectionID);
deviceID = ID;
return resolve(ID);
deviceID = id;
return resolve(id);
},
});
});
Expand All @@ -38,7 +38,7 @@ function setDeviceID() {
throw new Error(existingDeviceID);
})
.then(generateDeviceID)
.then((uniqueID) => {
.then((uniqueID: string) => {
Log.info('Got new deviceID', false, uniqueID);
Onyx.set(ONYXKEYS.DEVICE_ID, uniqueID);
})
Expand All @@ -47,9 +47,9 @@ function setDeviceID() {

/**
* Returns a string object with device info and uniqueID
* @returns {Promise<string>}
* @returns - device info with ID
*/
function getDeviceInfoWithID() {
function getDeviceInfoWithID(): Promise<string> {
return new Promise((resolve) => {
getDeviceID().then((currentDeviceID) =>
resolve(
Expand Down
Loading