search

aws / amazon-chime-sdk-js

A JavaScript client library for integrating multi-party communications powered by the Amazon Chime service.
Apache License 2.0
713 stars 475 forks source link

Fallback to -1 for browser majorVersion when version is null #2687

Closed michaelbachmann closed 1 year ago

michaelbachmann commented 1 year ago

Issue #: 1856

Description of changes: In ReactNative detect from detect-browser library returns null for version. When DefaultMessagingSession signs the websocket url, the sigv4 class calls Versioning.sdkUserAgentLowResolution which in turns calls DefaultBrowserBehavior.majorVersion. This should provide a default major version of -1 whenever detect returns null for version.

Testing:

Can these tested using a demo application? Please provide reproducible step-by-step instructions.

Unit test + validated could start messaging session in our react native app.

Checklist:

  1. Have you successfully run npm run build:release locally? Yes.
=============================== Coverage summary ===============================
Statements   : 100% ( 10854/10854 )
Branches     : 100% ( 4803/4803 )
Functions    : 100% ( 2062/2062 )
Lines        : 100% ( 10727/10727 )
================================================================================

  1. Do you add, modify, or delete public API definitions? If yes, has that been reviewed and approved? No.

  2. Do you change the wire protocol, e.g. the request method? If yes, has that been reviewed and approved? No.

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

xuesichao commented 1 year ago

@michaelbachmann Thanks for contributing to our SDK! GitHub action failed due to the lack of CHANGELOG, Could you please update the CHANGELOG for your change?

xuesichao commented 1 year ago

Right now we give a fall back value for entire browser when detect() returns a falsy value. But it could be better if we could add some null check to give a fallback value for browser.version and browser.os when their value is null. Would you like to add them? I closed my PR and let me know if you want me to make this change and I'll submit a new one. Thanks.

https://github.com/aws/amazon-chime-sdk-js/blob/0aabd5aa821051e02569a0fb77736224416cdf3b/src/browserbehavior/DefaultBrowserBehavior.ts#L11-L17

michaelbachmann commented 1 year ago

Right now we give a fall back value for entire browser when detect() returns a falsy value. But it could be better if we could add some null check to give a fallback value for browser.version and browser.os when their value is null. Would you like to add them? I closed my PR and let me know if you want me to make this change and I'll submit a new one. Thanks.

https://github.com/aws/amazon-chime-sdk-js/blob/0aabd5aa821051e02569a0fb77736224416cdf3b/src/browserbehavior/DefaultBrowserBehavior.ts#L11-L17

@xuesichao Thank you so much! Honestly, if you wouldn't mind I feel like it'd be a lot faster for you to do it, I actually don't even know how to update the change log lol. Thank you so much again for the help on this, were so excited to be able to use this library!

xuesichao commented 1 year ago

Close this PR because of https://github.com/aws/amazon-chime-sdk-js/pull/2688.