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

prioritize browser detection, remove os dependency #65

Merged
merged 1 commit into from
Jun 25, 2018

Conversation

DamonOehlman
Copy link
Owner

So this is a reasonably simple change, but one that should have a pretty big impact for people who have been hit with the code path which is returning a false positive on node detection (see #54).

It changes two things:

  1. We do browser detection if at all possible (i.e. navigator is defined). This then falls back to detecting the node environment if that isn't available.

  2. Removes the require('os') import for os detection and simply uses process.platform instead. The information won't be as detailed and it's possible that a user of this module may still want to opt to do their own check using the os module. Given that the majority use case is web browser detection though I think that makes sense. Should stop browserify bringing in any shims for the os module also (which is a good thing).

Because of point 2 though, I believe we should do a major version bump to 3.0.0 to ensure that no-one that is using detect-browser in an isomorphic way is caught out unexpectedly.

@5punk PTAL.

/cc @shengshiqi @markgoho-EDT

@DamonOehlman
Copy link
Owner Author

@5punk Hey mate - just wondering if you've had time to look at this. I'm keen to create a 3.0 version soon as there is another feature that has been requested that would definitely need a major version bump to be included.

@5punk
Copy link
Collaborator

5punk commented Jun 21, 2018

I'll take a look at this today.
Sorry for being AWOL.

@5punk
Copy link
Collaborator

5punk commented Jun 21, 2018

I would feel a little to see a unit test for this use case. Maybe in a future rewrite we can use webpack/rollup, typescript and jest/ava

@vstets
Copy link

vstets commented Jun 25, 2018

Hello guys,

Please complete this PR - we are waiting on it.

@DamonOehlman
Copy link
Owner Author

@vstets The PR will be merged shortly. I'll add some comments on your issues / PRs shortly so we can action that also.

@DamonOehlman DamonOehlman merged commit 0d871e1 into master Jun 25, 2018
@DamonOehlman DamonOehlman deleted the damon-fix-node-os-check branch June 25, 2018 08:34
tomonari-t pushed a commit to tomonari-t/detect-browser that referenced this pull request Jan 31, 2019
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