Skip to content

Add missing 'platform' field#82

Open
xloem wants to merge 1 commit into
defunctzombie:masterfrom
xloem:master
Open

Add missing 'platform' field#82
xloem wants to merge 1 commit into
defunctzombie:masterfrom
xloem:master

Conversation

@xloem

@xloem xloem commented Jan 13, 2018

Copy link
Copy Markdown

No description provided.

@zenflow

zenflow commented Jan 13, 2018

Copy link
Copy Markdown
Contributor

Resolves issue #55?

@xloem

xloem commented Jan 13, 2018

Copy link
Copy Markdown
Author

So it does! Shoulda checked the issues first.

@zenflow

zenflow commented Jan 14, 2018

Copy link
Copy Markdown
Contributor

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

@CMCDragonkai

CMCDragonkai commented May 14, 2018

Copy link
Copy Markdown

So what's stopping this from being merged?

@calvinmetcalf

Copy link
Copy Markdown
Collaborator

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

CMCDragonkai commented May 14, 2018

Copy link
Copy Markdown

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

@xloem

xloem commented May 14, 2018

Copy link
Copy Markdown
Author

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

calvinmetcalf commented May 14, 2018 via email

Copy link
Copy Markdown
Collaborator

@CMCDragonkai

CMCDragonkai commented May 31, 2018

Copy link
Copy Markdown

@calvinmetcalf Can we get this merged?

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

@calvinmetcalf

Copy link
Copy Markdown
Collaborator

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

@CMCDragonkai

CMCDragonkai commented Jun 1, 2018 via email

Copy link
Copy Markdown

@zenflow

zenflow commented Jun 1, 2018

Copy link
Copy Markdown
Contributor

@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

zenflow commented Jun 1, 2018

Copy link
Copy Markdown
Contributor

@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

Copy link
Copy Markdown

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

Copy link
Copy Markdown
Collaborator

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

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.

4 participants