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

fix(amazon-cognito-identity-js): specify the correct userAgent/deviceName when remembering devices (React Native) #10724

Merged

Conversation

Samaritan1011001
Copy link
Contributor

@Samaritan1011001 Samaritan1011001 commented Nov 29, 2022

Description of changes

API: Auth.rememberDevice & Auth.listDevices
Existing code only depended on the navigator !== undefined to check if it's the target device is a browser or not.
React Native initializes this navigator object leaving the above check invalid and inaccurate.
Here we make use of polyfilled navigator.product to determine if it's ReactNative and update the device name accordingly

Issue #, if available

Description of how you validated changes

Ran a sample React Native Android app containing the remember and list devices feature turned on. List Devices API returned the device with the name 'react-native'

Integ test for this fix running on iOS only - https://github.com/aws-amplify/amplify-js-samples-staging/pull/491
CircleCI tests passing on a fix branch - https://app.circleci.com/pipelines/github/aws-amplify/amplify-js/17151/workflows/0997c593-9fe9-4e58-b157-9efe91110e7d

Checklist

  • PR description included
  • yarn test passes
  • Tests are changed or added
  • Relevant documentation is changed or added (and PR referenced)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@lgtm-com
Copy link

lgtm-com bot commented Nov 29, 2022

This pull request introduces 1 alert when merging 60e2422 into fa7d6c6 - view on LGTM.com

new alerts:

  • 1 for Variable not declared before use

Heads-up: LGTM.com's PR analysis will be disabled on the 5th of December, and LGTM.com will be shut down ⏻ completely on the 16th of December 2022. Please enable GitHub code scanning, which uses the same CodeQL engine ⚙️ that powers LGTM.com. For more information, please check out our post on the GitHub blog.

@codecov-commenter
Copy link

codecov-commenter commented Nov 29, 2022

Codecov Report

Merging #10724 (8a50266) into main (fb85783) will decrease coverage by 0.00%.
The diff coverage is 71.42%.

@@            Coverage Diff             @@
##             main   #10724      +/-   ##
==========================================
- Coverage   86.16%   86.15%   -0.01%     
==========================================
  Files         197      197              
  Lines       18371    18376       +5     
  Branches     3907     3910       +3     
==========================================
+ Hits        15829    15832       +3     
- Misses       2468     2470       +2     
  Partials       74       74              
Impacted Files Coverage Δ
...ages/amazon-cognito-identity-js/src/CognitoUser.js 79.06% <71.42%> (-0.12%) ⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@Samaritan1011001 Samaritan1011001 marked this pull request as ready for review November 30, 2022 00:35
@Samaritan1011001 Samaritan1011001 requested a review from a team as a code owner November 30, 2022 00:35
@Samaritan1011001 Samaritan1011001 changed the title fix(amazon-cognito-identity-js): specify the right userAgent/deviceName when remembering devices (React Native) fix(amazon-cognito-identity-js): specify the correct userAgent/deviceName when remembering devices (React Native) Nov 30, 2022
Copy link
Contributor

@elorzafe elorzafe left a comment

Choose a reason for hiding this comment

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

Thanks @Samaritan1011001

Need to add a check that navigator is not undefined

packages/amazon-cognito-identity-js/src/CognitoUser.js Outdated Show resolved Hide resolved
elorzafe
elorzafe previously approved these changes Nov 30, 2022
Copy link
Contributor

@elorzafe elorzafe left a comment

Choose a reason for hiding this comment

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

Looks good!

Thanks @Samaritan1011001 🎖️

jimblanc
jimblanc previously approved these changes Dec 1, 2022
Copy link
Contributor

@jimblanc jimblanc left a comment

Choose a reason for hiding this comment

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

Looks good!

@Samaritan1011001 Samaritan1011001 requested a review from a team as a code owner December 15, 2022 21:56
AllanZhengYP
AllanZhengYP previously approved these changes Dec 15, 2022
@@ -1065,6 +1065,13 @@ jobs:
steps:
- integ_test_rn_ios

integ_rn_ios_device_tracking:
Copy link
Contributor

Choose a reason for hiding this comment

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

You need to add this also on the workflow section

workflows:
and also required for deploy on
- deploy:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants