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

feat: add normalizers (byEngine and toDesktop) #511

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

ylemkimon
Copy link

@ylemkimon ylemkimon commented Jul 23, 2020

  • docs: update mobileToDesktop documentation to match its behavior
  • feat: add normalizers
  • feat: add byEngine and toDesktop normalizers

Fixes #508, fixes #510.

README.md Outdated Show resolved Hide resolved
will return `and_chr` with the version of Chromium they are based on.
Note Edge and Opera is not normalized. Gecko-based browsers are
also normalized to Firefox, e.g., KaiOS Browser will return `and_ff`.
* `toDesktop`: Normalize mobile browsers to desktop browsers.
Copy link
Author

Choose a reason for hiding this comment

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

I've named it toDesktop instead of mobileToDesktop as it may be confused with the existing option.

Copy link
Member

Choose a reason for hiding this comment

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

What is the difference between normalizers: ['toDesktop'] and mobileToDesktop: true?

Copy link
Author

@ylemkimon ylemkimon Jul 28, 2020

Choose a reason for hiding this comment

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

@ai See #510 and 2ca9e82. mobileToDesktop only resolves the version part using desktop data. For example:

$ node cli.js "last 5 and_chr versions"
and_chr 81
$ node cli.js "last 5 chrome versions"
chrome 83
chrome 81
chrome 80
chrome 79
chrome 78
$ node cli.js --mobile-to-desktop "last 5 and_chr versions"
and_chr 83
and_chr 81
and_chr 80
and_chr 79
and_chr 78
$ node cli.js --normalizers=toDesktop "last 5 and_chr versions"
chrome 81
$ node cli.js --normalizers=toDesktop --mobile-to-desktop "last 5 and_chr versions"
chrome 83
chrome 81
chrome 80
chrome 79
chrome 78

Copy link
Member

@ai ai Jul 28, 2020

Choose a reason for hiding this comment

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

@JLHwung what do you think if we change mobileToDesktop behavior? On last 5 and_chr versions we will return chrome 78-83 instead of and_chr 78-83. Now it will affect Babel?

Copy link
Contributor

@JLHwung JLHwung Jul 28, 2020

Choose a reason for hiding this comment

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

Internally Babel maps and_chr to chrome and it seems to me this PR moves this process to browserslists. I don't think it will affect Babel.

Another thoughts about mobileToDesktop: AFAIK opera_mob is mapped to opera, this is incorrect because they are, if not always, frequently based on different chromium versions. Can we incorporate data from https://github.com/mdn/browser-compat-data/blob/master/browsers/opera_android.json ? This can help preset-env on supporting opera_mob. Surely we can address it in a different PR.

Copy link
Author

@ylemkimon ylemkimon Jul 29, 2020

Choose a reason for hiding this comment

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

I've added 'op_mob 46': 63 to normalizedVersions (byEngine). I think removing it from desktopNames (mobileToDesktop) would be out of scope for this PR.

@ylemkimon ylemkimon changed the title feat: add normalizers and keepOriginal option feat: add normalizers (toChromium and toDesktop) and keepOriginal option Jul 26, 2020
README.md Outdated
also normalized to Firefox, e.g., KaiOS Browser will return `and_ff`.
* `toDesktop`: Normalize mobile browsers to desktop browsers.
For instance, Browserslist will return `chrome 20` on `and_chr 20`
* `keepOriginal`: Keep the original browser when normalizing with `normalizers`
Copy link
Member

Choose a reason for hiding this comment

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

Can you explain why do we need this option?

Copy link
Author

Choose a reason for hiding this comment

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

In case tools need or can process unnormalized data. I don't have any specific use case yet.

Copy link
Member

Choose a reason for hiding this comment

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

Let’s remove it. I think the origin mobileToDesktop should be replaced.

Copy link
Author

Choose a reason for hiding this comment

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

@ai

I think the origin mobileToDesktop should be replaced.

I agree it'd be the best course of action, but it'd be a breaking change.

@ylemkimon ylemkimon changed the title feat: add normalizers (toChromium and toDesktop) and keepOriginal option feat: add normalizers (toChromium and toDesktop) Jul 29, 2020
@ylemkimon ylemkimon changed the title feat: add normalizers (toChromium and toDesktop) feat: add normalizers (byEngine and toDesktop) Jul 29, 2020
@ai ai mentioned this pull request Sep 20, 2020
@ai ai changed the base branch from master to main December 1, 2020 04:56
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.

mobileToDesktop doesn't change the browser name Option to return Chrome versions for Chromium-based browsers
3 participants