A life beyond browserify (and potentially CommonJS)

[DamonOehlman]

When I first wrote detect-browser it was really just to meet my own needs for browser detection in a simple app I wrote. That app used browserify and worked well. However, as more module loaders have hit the scene and we have ES6 modules available to us now I felt it was important to consider to update detect-browser (which has also been catalysed by others - see #23 /cc @ashubham).

There are few things that the current implementation of detect-browser doesn't do quite right:

1. It is implemented in a very browserify specific way (using the browser tag in the package.json file to specify where the browser specific logic lives). This really isn't necessary as we can make some determinations at runtime as to whether we are running in a node or browser environment through the existence of the navigator and process constructors (though things like jsdom might have trouble).

2. The detection logic occurs as the module is loaded. While this is quite convenient, it is generally bad practice and with ES6 modules have a different module resolution order (due to static analysis) to node modules we are likely to see this cause problems.

For these reasons I'm suggesting a refactoring of the files, and also of the API. The changes to the file structure can be seen here, and a simple code example of the API is shown below (equivalent to the example in the README):

const { detect } = require('detect-browser');
const browser = detect();

// handle the case where we don't detect the browser
if (browser) {
  console.log(browser.name);
  console.log(browser.version);
}

While the example above uses some ES6 sure, the module itself is still written in pure ES6 so browserify can still be used to wrap the module up and have it run within an app that needs to target IE (and other older browsers).

I'd like to get this PR across the line before proceeding on implementing other features such as platform / os detection which have been discussed in issues #44 and #45.

@5punk @ashubham @tomekwi General thoughts?

[rotatordisk92]

That makes a lot of sense.

I'm glad we are opening up our doors to all bundlers.

• I was hoping a refactor would address the issue of multiple matches and having the most accurate/unique browser regexes be at the top of the array.
• We probably should use ES6, and use the reduce() or filter() to get the match we want.
• Then, we can use the module flag in package.json to really allow module bundlers to use this module in the most efficient and optimized way.

Maybe it's just me.

[rotatordisk92]

👍

[DamonOehlman]

@5punk Changes up integrating the detectOS changes you made (thanks again for this). I did refactor the regexes to match the previous syntax but agree that your code with name and rule had better readability in the parsing so introduced a buildRules which takes a tuple definition format and turns it into your { name, rule } format.

With regards to your other comments:

I was hoping a refactor would address the issue of multiple matches and having the most accurate/unique browser regexes be at the top of the array.

Interesting, and good point we should probably do this. Would like to know what false positives people are hitting.

We probably should use ES6, and use the reduce() or filter() to get the match we want.

I have to stop myself from writing ES6 every time I touch this package as I expect it should probably still work with older versions of IE (there is a rule in there for detecting IE7) and I expect older versions of webkit (yes, pre blink) that exist on Android devices don't support newer JS either :(

Then, we can use the module flag in package.json to really allow module bundlers to use this module in the most efficient and optimized way.

Tell me more - you have my interest :)

[DamonOehlman]

Slightly shorter form of what you had previously, but pretty much the same.

[DamonOehlman]

For the sake of consistency, I return something useful for node.

[DamonOehlman]

A function for both getBrowserRules and getOperatingSystemRules mainly because I can see this translating well to a static method on a class at some point in the future (when we can have all the nice ES6 things).

[marcelgood]

What about updated TypeScript type definitions? I just updated to v2.0.0, but quickly realized that there is no corresponding @types/detect-browser, so my app fails because I'm still coded against the old API. Would be nice to get updated TypeScript definitions as well. Going back to v1 for now.

[DamonOehlman]

@marcelgood Sure thing. I've tracked back the original authorship of the @types/detect-browser definitions in DefinitelyTyped to @rogierschouten.

Just wondering if you folks might be able to work together to coordinate an update to that repo? I honestly didn't know definitions for detect-browser had been written; very pleased that they have been though. If not I'll try and get to it as soon as I can.

[tiagoefmoraes]

@marcelgood @DamonOehlman Typings on @types/detect-browser have been updated to v2.0 API.

[DamonOehlman]

@tiagoefmoraes Awesome - thanks heaps!