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

process.platform #55

Open
calvinmetcalf opened this Issue Mar 14, 2016 · 12 comments

Comments

Projects
None yet
6 participants
@calvinmetcalf
Copy link
Collaborator

calvinmetcalf commented Mar 14, 2016

we should probably set this to 'browser', thoughts ?

@defunctzombie

This comment has been minimized.

Copy link
Owner

defunctzombie commented Mar 19, 2016

Seems simple enough.

@defunctzombie

This comment has been minimized.

Copy link
Owner

defunctzombie commented Mar 19, 2016

Tho then someone will complain saying it should be the user agent :p

@calvinmetcalf

This comment has been minimized.

Copy link
Collaborator

calvinmetcalf commented Mar 19, 2016

well they'll probably really want it to be firefox or chrome and totally be
to young to remember the history of user agents

On Sat, Mar 19, 2016 at 2:29 AM Roman Shtylman notifications@github.com
wrote:

Tho then someone will complain saying it should be the user agent :p


You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
#55 (comment)

@isiahmeadows

This comment has been minimized.

Copy link

isiahmeadows commented Nov 9, 2016

@defunctzombie @calvinmetcalf

I feel process.platform should just be "browser". It's a single API (complete with entirely different path handling and file management), and testing for user agent is hardly useful at best, especially when you're just doing basic environment detection. Asking for process.platform to reflect the user agent is like asking it to be "debian", "fedora", etc. in Linux distros instead of just "linux" in Node.js itself.

@CMCDragonkai

This comment has been minimized.

Copy link

CMCDragonkai commented Nov 6, 2017

I vote for browser as well. For now I have to check for process.browser === true instead of just checking process.platform.

@zenflow

This comment has been minimized.

Copy link
Contributor

zenflow commented Nov 6, 2017

@CMCDragonkai if (process.browser === true) { is not any more code than if (process.platform === 'browser') { ... ? Not sure what case process.platform is simpler in... ?

Also the === true bit is redundant (i.e. use just if (process.browser) {) unless you need the result to always be an actual Boolean and not just truthy-falsy.

@CMCDragonkai

This comment has been minimized.

Copy link

CMCDragonkai commented Nov 7, 2017

It just means I can use a switch case expression on process.platform instead of checking against a browser property and the platform property.

@calvinmetcalf

This comment has been minimized.

Copy link
Collaborator

calvinmetcalf commented Nov 7, 2017

pull requests are accepted wink wink

@vmx

This comment has been minimized.

Copy link

vmx commented Mar 12, 2018

I've made a module to make the process.platform resemble the Node.js ones. This is useful for testing when you test on Node.js as well as on Browsers and need to skip tests based on the platform: https://github.com/ipfs/browser-process-platform/

@isiahmeadows

This comment has been minimized.

Copy link

isiahmeadows commented Mar 12, 2018

@vmx I would consider it unusual to need that. Browsers are sufficiently uniform across operating systems (we only need to remap key bindings, not paths, etc.) that for most use cases, it's pointless to draw such a distinction. Also, when it comes to skipping tests like that, I typically prefer controlling how I invoke the runner rather than in the tests themselves.

@vmx

This comment has been minimized.

Copy link

vmx commented Mar 12, 2018

@isiahmeadows It's indeed uncommon. In my case some HTTP API is tested which is not fully implemented on all platforms. Hence I skip tests based on the platform the Browser is running on.

@CMCDragonkai

This comment has been minimized.

Copy link

CMCDragonkai commented May 14, 2018

@defunctzombie We have a PR for the platform field. #82

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