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

Add is.propertyKey #138

Merged
merged 9 commits into from Sep 10, 2021
Merged

Add is.propertyKey #138

merged 9 commits into from Sep 10, 2021

Conversation

PopGoesTheWza
Copy link
Contributor

@PopGoesTheWza PopGoesTheWza commented Sep 7, 2021

Fixes #136

@PopGoesTheWza
Copy link
Contributor Author

@sindresorhus older version of Typescript may not include Symbol in the PropertyKey definition. Would that be an issue?

source/index.ts Outdated Show resolved Hide resolved
source/index.ts Outdated Show resolved Hide resolved
test/test.ts Outdated Show resolved Hide resolved
@sindresorhus
Copy link
Owner

You need to add it to the readme.

source/index.ts Show resolved Hide resolved
@sindresorhus sindresorhus changed the title feat: is.propertyKey Add is.propertyKey Sep 9, 2021
Repository owner deleted a comment from sindresorhus Sep 9, 2021
source/index.ts Outdated Show resolved Hide resolved
test/test.ts Outdated Show resolved Hide resolved
Copy link
Collaborator

@gioragutt gioragutt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test needs a fix, otherwise looks good. Never knew PropertyKey was a built-in type, always used keyof any instead. Thanks for the info :)

readme.md Outdated Show resolved Hide resolved
Co-authored-by: Giora Guttsait <giora111@gmail.com>
@PopGoesTheWza
Copy link
Contributor Author

Never knew PropertyKey was a built-in type, always used keyof any instead.

Ouch! I used string | number before venturing in the Typescript standard libraries (which is full of good information). Support for symbol keys was added recently to Typescript.

source/index.ts Outdated Show resolved Hide resolved
Copy link
Collaborator

@gioragutt gioragutt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment about the comment wasn't fixed. I believe this would be the last change

@sindresorhus sindresorhus merged commit d2f98e4 into sindresorhus:main Sep 10, 2021
@gioragutt
Copy link
Collaborator

@PopGoesTheWza thanks for your contribution bro 🤘🏻

@PopGoesTheWza
Copy link
Contributor Author

thanks for your contribution bro

On a totally irrelevant and lighthearted matter, I viscerally hate the term "bro". I can do with "dude" or "mate" though... ;)

@gioragutt
Copy link
Collaborator

gioragutt commented Sep 10, 2021

@PopGoesTheWza I swear I thought about it ASA I posted the comment 😅 Thanks for pointing it out 🙏🏻

@PopGoesTheWza PopGoesTheWza deleted the issue-136 branch September 10, 2021 10:12
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.

Proposal: is.propertyKey(value)
3 participants