-
Notifications
You must be signed in to change notification settings - Fork 4
refactor: streamline device identifier handling and error management #230
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
Conversation
| attributes: { endpoint: '/backup/v2/devices' }, | ||
| }); | ||
| return left(error); | ||
| return { error: error }; |
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.
[Non blocking] You don't really need to write error twice. You can use this eslint rule to show a warning: https://eslint.org/docs/latest/rules/object-shorthand
| export type FetchDeviceProps = { deviceIdentifier: DeviceIdentifierDTO } | { uuid: string } | { legacyId: string }; | ||
|
|
||
| async function getDeviceByProps(props: FetchDeviceProps): Promise<Either<Error, Device | null>> { | ||
| async function getDeviceByProps(props: FetchDeviceProps) { |
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.
[Non blocking] Just to give some information. We found that maybe this is no longer necessary since this is just a backup to recover to the device identifier of the user in case the app is uninstalled. However, the backup paths are lost anyway, so this doesn't provide a solution.
|
|
||
| const legacyId = configStore.get('deviceId'); | ||
| const savedUUID = configStore.get('deviceUUID'); | ||
| logger.debug({ |
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.
Aren't these logs useful to track if the device is migrated or in linux are we logging already the requests so we can see the that?
|
|
||
| const device = createNewDeviceEither.getRight(); | ||
| const user = DependencyInjectionUserProvider.get(); | ||
| user.backupsBucket = device.bucket; |
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.
Technically backupsBucket is always defined so we shouldn't do this anymore. We had the same code on windows and we deleted it. Check if that's true and you can simplify 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.
I ran some tests and confirmed that the backupsBucket does not exist for new users until they create their first device, so this line is still necessary.
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.
Oh shit, then we have a bug on windows. Thanks for the info :)
|


What is Changed / Added
Why