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

Fix colornames default export type definition #35354

Merged
merged 4 commits into from May 14, 2019

Conversation

Projects
None yet
5 participants
@blturner
Copy link
Contributor

commented May 10, 2019

Please fill in this template.

  • Use a meaningful title for the pull request. Include the name of the package modified.
  • Test the change in your own code. (Compile and run.)
  • Add or edit tests to reflect the change. (Run with npm test.)
  • Follow the advice from the readme.
  • Avoid common mistakes.
  • Run npm run lint package-name (or tsc if no tslint.json is present).

Select one of these and delete the others:

If changing an existing definition:

  • Provide a URL to documentation or source code which provides context for the suggested changes: https://github.com/timoxley/colornames/blob/1.1.1/index.js#L24
  • Increase the version number in the header if appropriate.
  • If you are making substantial changes, consider adding a tslint.json containing { "extends": "dtslint/dt.json" }.

The exported default function does not return a Color object, it returns the hexadecimal string value of the resolved color name. The type definition assumes it is a Color object.

I encountered this error when attempting to use the library in an existing typescript project using import.

In testing this change to the type definition, I did notice that using require did not cause any typescript errors:

const getHex = require("colornames");

But importing the library this way did:

import * as getHex from "colornames";

The changes to the type definition in this PR work with both of the above examples.

@typescript-bot

This comment has been minimized.

Copy link
Contributor

commented May 10, 2019

@blturner Thank you for submitting this PR!

🔔 @manuth - please review this PR in the next few days. Be sure to explicitly select Approve or Request Changes in the GitHub UI so I know what's going on.

If no reviewer appears after a week, a DefinitelyTyped maintainer will review the PR instead.

@manuth
Copy link
Contributor

left a comment

This PR looks perfect to me.
I just compared it to the API of colornames and it just matches perfectly.

Please consider to have a look at the comment I wrote during this review.

Show resolved Hide resolved types/colornames/colornames-tests.ts Outdated
@manuth

manuth approved these changes May 10, 2019

Copy link
Contributor

left a comment

Looks all fine to me.
Thanks for your contribution!

@typescript-bot typescript-bot moved this from Waiting for Reviewers to Check and Merge in Pull Request Status Board May 10, 2019

@typescript-bot

This comment has been minimized.

Copy link
Contributor

commented May 10, 2019

A definition owner has approved this PR ⭐️. A maintainer will merge this PR shortly. If it shouldn't be merged yet, please leave a comment saying so and we'll wait. Thank you for your contribution to DefinitelyTyped!

@sandersn sandersn merged commit 5f8fe32 into DefinitelyTyped:master May 14, 2019

2 checks passed

DefinitelyTyped.DefinitelyTyped Build #20190510.38 succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

Pull Request Status Board automation moved this from Check and Merge to Done May 14, 2019

@typescript-bot

This comment has been minimized.

Copy link
Contributor

commented May 14, 2019

I just published @types/colornames@1.1.1 to npm.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.