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

Inline union of CSS colors for TypeScript type #62

Merged
merged 3 commits into from
Oct 4, 2020

Conversation

Richienb
Copy link
Contributor

@Richienb Richienb commented Sep 5, 2020

Fixes #58

// @sindresorhus

Signed-off-by: Richie Bendall <[email protected]>
@sindresorhus
Copy link
Member

Put the elements of separate lines with the line starting with the pipe character.

Signed-off-by: Richie Bendall <[email protected]>
@Richienb
Copy link
Contributor Author

Richienb commented Sep 5, 2020

@sindresorhus Done!

@Qix-
Copy link
Member

Qix- commented Sep 5, 2020

Meh, this seems needless. It's a tiny dependency. The code is going to be here anyway - we're just shifting it around, with the added negative that if the upstream dependency updates, we'll be out of sync.

This isn't solving any problems, aside from placating the woes of those who have not had to maintain the divergence of Typescript and Javascript over the last 5+ years...

If this is what we really want, then PR looks good. I just think this is silly, however.

@Richienb
Copy link
Contributor Author

Richienb commented Sep 5, 2020

upstream dependency updates

Do you mean the CSS spec?

IMO, we should be keeping the dependency in here but I'm not making the choices.

@Qix-
Copy link
Member

Qix- commented Sep 5, 2020

Do you mean the CSS spec?

That's the obvious case, but any small errors in the type definitions (since we do not maintain them) or any changes in the typescript itself to better help typescript users, etc. Could be a number of things.

This change exists because someone is asking for a few less HTTP requests during npm i. It's not reducing code size by much since all of the color names are being copied over anyway.

It's bikeshedding but it's just.. not the right solution. It is the package manager/npm equivalent of micro-optimizations.

@sindresorhus sindresorhus changed the title Inline union of CSS colors Inline union of CSS colors for TypeScript type Sep 5, 2020
@sindresorhus
Copy link
Member

@Qix- I said yes to this as the CSS colors are very unlikely to change. Also, the dependency we're using is maintained by DefinitelyTyped, not the color-name maintainer, and it is not automatically kept in sync. So there's not much benefit of depending on this dependency.

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.

Why is @types/color-name a production dependency?
3 participants