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

Version numbers as Integer? #398

Closed
canvasplay opened this issue Oct 16, 2019 · 4 comments
Closed

Version numbers as Integer? #398

canvasplay opened this issue Oct 16, 2019 · 4 comments

Comments

@canvasplay
Copy link

Why when retrieving os/browser/... version numbers are typed as string? Why not an Integer?
This question just came to my mind... Shouldn't it be the expected behavior?
Is more than probably that one using this library may need at some time to perform < or > comparisons using those values.

This looks very desirable from my point of view, but it also seems too much obvious...
Is this intentional? I don't understand all the regex in here... maybe there is a reason ;)

I just started to investigate it deep in the code and found this line:

browser.major = util.major(browser.version); // deprecated

This is where I though the type conversion could be implemented...
But may I ask why the "deprecated" annotation in all "major" related lines?

Looks like this tool is focusing on being a regex-only tool. Isn't it?

Thanks in advance.

@rtarnaud
Copy link

That's because some versions are labeled as x.y.z like in semver.

@canvasplay
Copy link
Author

canvasplay commented Dec 12, 2019

That's because some versions are labeled as x.y.z like in semver.

Can't semver be parsed as integers?
I'm just arguing that having this data as an object would be useful, something like:

version: {
  label: 'x.y.z',
  major: x,
  minor: y,
  patch: z
}

@faisalman
Copy link
Owner

Hi @canvasplay, regarding the type of version, obviously it has to be string because the property holds the complete value of "x.y.z-a.b.c". Instead, there was initially also a major property that can be used as comparison tool (albeit not having integer type because there are numerous OS/browser that also have string value in their version like XP, Vista, or Wii), but decided to only focus in extracting the full-length version and hand over the work of splitting version numbers to major, minor, patch outside of this library. Although, maybe sometime in the upcoming releases it is possible to also serves those data, given the code is still inside of this library:

major : function (version) {
return typeof(version) === STR_TYPE ? version.replace(/[^\d\.]/g,'').split(".")[0] : undefined;
},

@faisalman
Copy link
Owner

Until now, the responsibility to properly convert string to number before comparing versions still lies on each app that consume this library.. but given how '100' > '52' can be FALSE but '520' > '52' is TRUE in JavaScript, as what was found in this case involving @slackhq: webcompat/web-bugs#67866 can be a real issue when Firefox /Chrome (which is now at 86 and 89 respectively) finally pushing out version 100 🤔

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants