search

defunctzombie / node-process

process information for node.js and browsers
MIT License
122 stars 62 forks source link

Add missing 'platform' field #82

Open xloem opened 6 years ago

zenflow commented 6 years ago

Resolves issue #55?

xloem commented 6 years ago

So it does! Shoulda checked the issues first.

zenflow commented 6 years ago

Lol! Lucky you then, it looks like this PR is already approved

CMCDragonkai commented 6 years ago

So what's stopping this from being merged?

calvinmetcalf commented 6 years ago

ok I'm at a conf this week, but will be back next week, my only concert is just things looking for process.platform to be undefined or something annoying like that so I want to be in a place that i can revert if need be

CMCDragonkai commented 6 years ago

If they polyfilled process. I'm thinking they expect the platform property to exist. Since its part of the API of process.

xloem commented 6 years ago

Hey, I don't remember anymore what I wanted this for, but if you're concerned about compatibility, just update the major version to 1.0.0 so old projects don't automatically upgrade to the change. See https://docs.npmjs.com/getting-started/semantic-versioning .

calvinmetcalf commented 6 years ago

this is something that isn't imported directly but is instead is pulled in by browserify or webpack, so semvar doesn't really apply to this library

On Mon, May 14, 2018 at 3:03 PM xloem notifications@github.com wrote:

Hey, I don't remember anymore what I wanted this for, but if you're concerned about compatibility, just update the major version to 1.0.0 so old projects don't automatically upgrade to the change. See https://docs.npmjs.com/getting-started/semantic-versioning .

— You are receiving this because you commented.

Reply to this email directly, view it on GitHub https://github.com/defunctzombie/node-process/pull/82#issuecomment-388849921, or mute the thread https://github.com/notifications/unsubscribe-auth/ABE4n6v4WQfQY4XBysTe_wM-eDYiotZuks5tyZzGgaJpZM4RdZoC .

CMCDragonkai commented 6 years ago

@calvinmetcalf Can we get this merged?

Also semver can still be used for devDependencies, which is how I use this library.

calvinmetcalf commented 6 years ago

@CMCDragonkai most people don't use the library that way is the issue

CMCDragonkai commented 6 years ago

What evidence do you have that adding a platform variable will adversely affect existing users?

On 31 May 2018 22:42:35 GMT+10:00, Calvin Metcalf notifications@github.com wrote:

@CMCDragonkai most people don't use the library that way is the issue

-- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: https://github.com/defunctzombie/node-process/pull/82#issuecomment-393517441

-- Sent from my Android device with K-9 Mail. Please excuse my brevity.

zenflow commented 6 years ago

@calvinmetcalf isn't the version of this pkg locked to this major version (at least for webpack users) here: https://github.com/webpack/node-libs-browser/blob/master/package.json

Don't know about browserify, parcel, etc.

zenflow commented 6 years ago

@CMCDragonkai I may be off base, but I think the possibility that a bit of logic expects process.platform === undefined in the browser is the worry?

CMCDragonkai commented 6 years ago

I was asking for evidence of libraries and applications that do that. And compare it to those that do not. And we can see what it costs to ask them to change.

calvinmetcalf commented 6 years ago

yeah my worry is there could be either if (!process.platform) or what @zenflow said so I don't want to accidentally break a bunch of peoples code, I also don't really want to do a major version bump just because of how this library is used, a major version bump would basically end up being a fork with some people on the old version and some people on the new one