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

Marks supports-color as an *optional* peer dependency #727

Merged
merged 1 commit into from
May 19, 2020

Conversation

arcanis
Copy link
Contributor

@arcanis arcanis commented Oct 16, 2019

This diff adds supports-color as an optional peer dependency. Optional peer dependencies don't trigger warnings when users omit them. They are supported by all package managers, including npm.

Adding these lines will prevent package managers from incorrectly hoisting supports-color in a way that would prevent debug from accessing it.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 87.692% when pulling 3c9da46 on arcanis:patch-1 into 5c7c61d on visionmedia:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage remained the same at 87.692% when pulling 3c9da46 on arcanis:patch-1 into 5c7c61d on visionmedia:master.

@Qix-
Copy link
Member

Qix- commented Oct 17, 2019

It was optional and many people decided not to read the documentation on how to use it.

@arcanis
Copy link
Contributor Author

arcanis commented Oct 17, 2019

@Qix- Can you detail? I'm not sure I follow.

Note that I'm not talking about optional dependencies, or peer dependencies. I'm talking about optional peer dependencies, which are a different type of dependency (like peer dependencies, except that they don't cause warnings when missing).

@arcanis
Copy link
Contributor Author

arcanis commented Dec 12, 2019

Ping @Qix-? This approach is a strict improvement with zero downsides, is there a blocker?

@Qix-
Copy link
Member

Qix- commented Dec 12, 2019

@arcanis Yes, I do OSS in my free time and I've been swamped at work lately. I need to vet this and do some research before I form a conclusion.

@arcanis
Copy link
Contributor Author

arcanis commented Dec 12, 2019

No worry 🙂

If that makes your work easier, here are some pointers (I'm the author of the RFC and two out of three implementations, feel free to ask for details as needed):

Various projects adopted it: create-react-app, typescript-eslint, Sindresorhus' packages, etc...

@arcanis
Copy link
Contributor Author

arcanis commented Jan 2, 2020

Ping? Just for context, I lead Yarn's development, and I can assure you there is no negative side effect to adding this field - neither on Yarn nor npm nor pnpm.

@arcanis
Copy link
Contributor Author

arcanis commented Jan 19, 2020

Ping. Starting from next week debug won't print colors on Yarn unless you merge and release this PR.

@arcanis
Copy link
Contributor Author

arcanis commented May 12, 2020

Ping 🙂

@Qix-
Copy link
Member

Qix- commented May 19, 2020

Ping. Starting from next week debug won't print colors on Yarn unless you merge and release this PR.

I'll merge and release, but let it be known: Yarn is not the standards comittee for package.json. I'll let it by this once, but I'm not going to add a bunch of extra meta information to the package.json just to appease each of the 10 different package managers.

These companies really need to get together and agree on a format because the package.json bloat over the last decade has been absurd.

@arcanis This is comment definitely directed towards your team. The Node.js ecosystem is way, way too bloated and complicated today - needlessly so.

@Qix- Qix- merged commit 09914af into debug-js:master May 19, 2020
@arcanis
Copy link
Contributor Author

arcanis commented May 19, 2020

These companies really need to get together and agree on a format because the package.json bloat over the last decade has been absurd.

Yarn is no company - we're a pure community-based open-source project. Additionally, this particular field got introduced two years ago after bringing both npm and pnpm people into the discussion.

I personally made the implementation PRs for both Yarn and npm, and made a few followup revisions to the design in order to make it easier for implementors such as you to start using it without it being painful for your users. I even went ahead and made the PRs on those projects to let you know about it before it had any chance to be noticed by your users - does that often happen?

Overall, I find your assessment quite unfair.

@zypA13510

This comment has been minimized.

@arcanis arcanis deleted the patch-1 branch May 19, 2020 09:40
@Qix-
Copy link
Member

Qix- commented May 19, 2020

@arcanis

Yarn is no company - we're a pure community-based open-source project.

It doesn't matter. You and NPM are two competing organizations who share a single data structure and cannot seem to agree to keep the scope in check or agree on the features. Further, nobody wants to break anyone in order to improve it, so every little thing that isn't quite right results in a new field or some weird bandaid patch - and us maintainers get hundreds of PRs to update it.

I even went ahead and made the PRs on those projects to let you know about it before it had any chance to be noticed by your users - does that often happen?

If my well-defined package breaks because you decided to change the way your project behaves, that's less to do with my project and more to do with either your project or the data structure you claim to control.

package.json has been flawed from the ground-up for years. After the npm@5 release, lockfiles, the endless torrent of bugs and headache that introduced (and the fact a large portion of it was implemented over a single weekend without much review in order to compete with Node.js proper's deadlines - without much of a code review or QA cycle), I have very little patience for any sort of modifications to package.json.

Now I have to go through the very, very delicate process of releasing debug because you have created a time-critical failure due to yet another change in package.json - a breaking bandaid change, at that.

Sorry if I seem annoyed. It's because I'm annoyed.


Thanks for the reminder @zypA13510; done.

@Qix-
Copy link
Member

Qix- commented May 19, 2020

Published as 4.2.0 under the beta dist-tag. Please test and make sure nothing has broken for you before I send to latest.

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

Successfully merging this pull request may close these issues.

5 participants