Skip to content

Conversation

HyewonKkang
Copy link
Contributor

Prerequisites

Type of Change

enhancement

Description

  • Added support for recognizing the 'Daum' app in user agent strings.
  • AS-IS: 'Daum' app was not identified.
  • TO-BE: User agents with 'Daum' are now accurately parsed.

application download link: google play, apple apps

Test

  • Added tests for 'Daum' user agent string.
  • Ran the full test suite successfully.

Impact

Breaking Changes: None
User Actions: No changes required, backward compatibility maintained

Other Info

Copy link
Owner

@faisalman faisalman left a comment

Choose a reason for hiding this comment

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

Hi @HyewonKkang, Thank you for contributing to this project!

  • Please commit only the source files and tests. You can omit the .mjs files as they will be autogenerated.
  • The value in the Browser enum should match with the browser name in the result:
import { UAParser } from 'ua-parser-js';
import { Browser } from 'ua-parser-js/enums';

const result = UAParser(navigator.userAgent);

// string comparison must be the same
if (result.browser.name == Browser.DAUM) {
  console.log('You are using Daum app');
}

@HyewonKkang
Copy link
Contributor Author

@faisalman
Thank you for reviewing quickly.
I've applied the review comments, so please check.

Copy link
Owner

@faisalman faisalman left a comment

Choose a reason for hiding this comment

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

Hi, thanks for following up, but this issue still hasn't been addressed:

* The value in the `Browser` enum should match with the browser name in the result:
// string comparison must be the same
if (result.browser.name == Browser.DAUM) {

You can change the captured string into, for example:

/(daum)apps[\/ ]([\w\.]+)/i,

So both of them will match

@HyewonKkang
Copy link
Contributor Author

HyewonKkang commented Dec 12, 2024

@faisalman
Thanks for the great suggestions 😄

Copy link
Owner

@faisalman faisalman left a comment

Choose a reason for hiding this comment

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

Change the value in the test files as well and run npm test to make sure that the change is all good.

Copy link
Owner

@faisalman faisalman left a comment

Choose a reason for hiding this comment

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

All seems good now, thanks!

@faisalman faisalman merged commit 4601953 into faisalman:master Dec 12, 2024
5 checks passed
nanalucky pushed a commit to nanalucky/ua-parser-js that referenced this pull request Feb 18, 2025
* Add Daum app user agent

---------

Co-authored-by: helen.one(강혜원)/kakao <helen.one@kakaocorp.com>
faisalman pushed a commit that referenced this pull request Aug 19, 2025
* Add Daum app user agent

---------

Co-authored-by: helen.one(강혜원)/kakao <helen.one@kakaocorp.com>
(cherry picked from commit 4601953)
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.

3 participants