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

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

Closed
wants to merge 1 commit into from

Conversation

michaelbachmann
Copy link

@michaelbachmann michaelbachmann commented Jun 21, 2023

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.

@michaelbachmann michaelbachmann requested a review from a team as a code owner June 21, 2023 23:43
@xuesichao
Copy link
Contributor

@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
Copy link
Contributor

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.

private readonly FALLBACK_BROWSER = {
type: 'browser',
name: 'unknown',
version: 'unknown',
os: 'unknown',
};
private readonly browser = detect() || this.FALLBACK_BROWSER;

@michaelbachmann
Copy link
Author

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.

private readonly FALLBACK_BROWSER = {
type: 'browser',
name: 'unknown',
version: 'unknown',
os: 'unknown',
};
private readonly browser = detect() || this.FALLBACK_BROWSER;

@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
Copy link
Contributor

Close this PR because of #2688.

@xuesichao xuesichao closed this Jun 26, 2023
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.

2 participants