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 #2688

Merged
merged 2 commits into from
Jun 26, 2023

Conversation

xuesichao
Copy link
Contributor

Issue #1856, #2674 :

Description of changes:
According to detect-library the detect() could return browser info with the value of version being null.

For example: https://github.com/DamonOehlman/detect-browser/blob/546e6f1348375d8a486f21da07b20717267f6c49/src/index.ts#L59-L65.

export class ReactNativeInfo
  implements DetectedInfo<'react-native', 'react-native', null, null> {
  public readonly type = 'react-native';
  public readonly name: 'react-native' = 'react-native';
  public readonly version: null = null;
  public readonly os: null = null;
}

In this case our majorVersion() will throw an error because it try execute null.split('.'). This change add a null check to avoid such case. And give it a fallback value -1.

Testing:

Can these tested using a demo application? Please provide reproducible step-by-step instructions.
No. The change has been covered by unit test.

Checklist:

  1. Have you successfully run npm run build:release locally?
    Yes

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

  3. 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 xuesichao merged commit e4e9c89 into main Jun 26, 2023
@xuesichao xuesichao deleted the fallback-major-version branch June 26, 2023 21:48
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