-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
mountiny
merged 20 commits into
Expensify:main
from
fvlvte:24877-migrate-device-lib-to-ts
Nov 9, 2023
Merged
Changes from 10 commits
Commits
Show all changes
20 commits
Select commit
Hold shift + click to select a range
2276094
Migrated src/libs/Device to TypeScript
fvlvte 8786f68
Added missing explicit type.
fvlvte 06a907c
Fixed JSDoc removal.
fvlvte ce892be
Refactored platform specific functions and typings.
fvlvte 660a8a3
Aligned to TS guidelines.
fvlvte a3cfbf6
Reflected PR comments.
fvlvte c47531d
Merge branch 'main' of github.com:fvlvte/App into 24877-migrate-devic…
fvlvte 02b524c
Suggested change from code review.
fvlvte 4ab974a
Merge branch 'main' into 24877-migrate-device-lib-to-ts
fvlvte e87a349
Applied linter changes and resolved conflicts.
fvlvte 90773e2
Fixed linter issues.
fvlvte ac5668c
Merge branch 'main' into 24877-migrate-device-lib-to-ts
fvlvte 686c0ad
Resolved linter issues.
fvlvte ae6f0aa
Merge branch 'main' into 24877-migrate-device-lib-to-ts
fvlvte 1008c6c
Conflicts resolved, TS-fixes, linter, prettier.
fvlvte 9162088
Updated expensify-common lib
fvlvte 7928fc1
Merge branch 'main' into 24877-migrate-device-lib-to-ts
fvlvte a468073
Merge branch 'main' into 24877-migrate-device-lib-to-ts
fvlvte ae92c8d
Merge branch 'main' into 24877-migrate-device-lib-to-ts
fvlvte ee562a9
Device lib fixes.
fvlvte File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
8 changes: 3 additions & 5 deletions
8
.../Device/generateDeviceID/index.desktop.js → .../Device/generateDeviceID/index.desktop.ts
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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; |
7 changes: 2 additions & 5 deletions
7
...ions/Device/generateDeviceID/index.ios.js → ...ions/Device/generateDeviceID/index.ios.ts
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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; |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
export type GenerateDeviceID = () => Promise<string>; | ||
9 changes: 5 additions & 4 deletions
9
...tions/Device/getDeviceInfo/getBaseInfo.js → ...tions/Device/getDeviceInfo/getBaseInfo.ts
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,8 +1,9 @@ | ||
import packageConfig from '../../../../../package.json'; | ||
import {GetBaseInfo} from "./types"; | ||
|
||
export default function getBaseInfo() { | ||
return { | ||
const getBaseInfo: GetBaseInfo = () => ({ | ||
app_version: packageConfig.version, | ||
timestamp: new Date().toISOString().slice(0, 19), | ||
}; | ||
} | ||
}); | ||
|
||
export default getBaseInfo; |
6 changes: 1 addition & 5 deletions
6
src/libs/actions/Device/getDeviceInfo/getOSAndName/index.native.ts
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
12 changes: 2 additions & 10 deletions
12
src/libs/actions/Device/getDeviceInfo/getOSAndName/index.ts
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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'; | ||
|
||
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; |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,5 @@ | ||
// 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; | ||
export type GetOSAndName = () => OSAndName; | ||
export type OSAndName = { | ||
device_name?: string; | ||
os_version?: string; | ||
} |
This file was deleted.
Oops, something went wrong.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
import getBaseInfo from './getBaseInfo'; | ||
import getOSAndName from './getOSAndName/index.native'; | ||
fvlvte marked this conversation as resolved.
Show resolved
Hide resolved
|
||
import {GetDeviceInfo} from './types'; | ||
|
||
const getDeviceInfo: GetDeviceInfo = () => ({ | ||
...getBaseInfo(), | ||
...getOSAndName(), | ||
os: 'Android', | ||
}); | ||
|
||
export default getDeviceInfo; |
This file was deleted.
Oops, something went wrong.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
import getBaseInfo from './getBaseInfo'; | ||
import getOSAndName from './getOSAndName/index.native'; | ||
fvlvte marked this conversation as resolved.
Show resolved
Hide resolved
|
||
import {GetDeviceInfo} from './types'; | ||
|
||
const getDeviceInfo: GetDeviceInfo = () => ({ | ||
...getBaseInfo(), | ||
...getOSAndName(), | ||
device_name: 'Desktop', | ||
}); | ||
|
||
export default getDeviceInfo; |
This file was deleted.
Oops, something went wrong.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
import getBaseInfo from './getBaseInfo'; | ||
import getOSAndName from './getOSAndName/index.native'; | ||
fvlvte marked this conversation as resolved.
Show resolved
Hide resolved
|
||
import {GetDeviceInfo} from './types'; | ||
|
||
const getDeviceInfo: GetDeviceInfo = () => ({ | ||
...getBaseInfo(), | ||
...getOSAndName(), | ||
os: 'iOS', | ||
}); | ||
|
||
export default getDeviceInfo; |
This file was deleted.
Oops, something went wrong.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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; |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,10 @@ | ||
import {OSAndName} from "./getOSAndName/types"; | ||
|
||
export type BaseInfo = { | ||
app_version: string; | ||
timestamp: string; | ||
}; | ||
|
||
export type GetDeviceInfo = () => DeviceInfo; | ||
export type DeviceInfo = BaseInfo & OSAndName & {os?: string, device_name?: string, device_version?: string}; | ||
export type GetBaseInfo = () => BaseInfo; |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.There was a problem hiding this comment.
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 oneThere was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
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?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
@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.
There was a problem hiding this comment.
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