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 to.keyword returning Object.prototype values #67

Merged
merged 1 commit into from
Apr 22, 2022
Merged

fix to.keyword returning Object.prototype values #67

merged 1 commit into from
Apr 22, 2022

Conversation

matias-la
Copy link
Contributor

$ node
Welcome to Node.js v16.9.1.
Type ".help" for more information.
> require('color-string').to.keyword(['constructor'])
[Function: Object]

The package had unexpected behavior when passing properties of Object.prototype to to.keyword. Using a null prototype for reverseNames fixes the issue.

@Qix-
Copy link
Owner

Qix- commented Apr 22, 2022

Mrh I was hoping we could just replace it with a Map but this package hasn't been updated to even es6 syntax yet so surely that would break something.

Thanks for the PR, LGTM.

@Qix- Qix- merged commit 937b690 into Qix-:master Apr 22, 2022
@matias-la
Copy link
Contributor Author

Thanks for responding so fast :) Would it be possible to make a release including this change?

@Qix-
Copy link
Owner

Qix- commented Apr 22, 2022

Yeah sorry was making sure the builds were green then got sidetracked. Releasing now.

@Qix-
Copy link
Owner

Qix- commented Apr 22, 2022

Released as 1.9.1, thanks again :)

@matias-la
Copy link
Contributor Author

Mrh I was hoping we could just replace it with a Map but this package hasn't been updated to even es6 syntax yet so surely that would break something.

I might send similar PRs for the color package. I'll use null prototypes and/or hasOwnProperty checks unless you prefer me to use Map instead.

@matias-la
Copy link
Contributor Author

Actually, it seems we won't need to use the color package anymore as we will develop an in-house alternative. Thanks anyway for the fast release!

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.

2 participants