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

Change tagged template literal argument type to accept unknown instead of just string #316

Merged
merged 4 commits into from Dec 28, 2018

Conversation

vanbujm
Copy link
Contributor

@vanbujm vanbujm commented Dec 23, 2018

Fixes #314. Changed Type to unknown as per @sindresorhus's request.

@vanbujm
Copy link
Contributor Author

vanbujm commented Dec 23, 2018

One thing to be aware of is unknown will break types for Typescript 2.x versions. Happy to submit a more complicated type def that uses any for older versions and unknown for newer versions if we want to support older TS versions.

@sindresorhus
Copy link
Member

One thing to be aware of is unknown will break types for Typescript 2.x versions.

That's fine. This change will be part of the next major release anyway.

types/index.d.ts Outdated Show resolved Hide resolved
@sindresorhus
Copy link
Member

Can you also update the Flow definition?

@vanbujm
Copy link
Contributor Author

vanbujm commented Dec 26, 2018

@sindresorhus, I had a crack at changing the types for flow.

However there are two flow tests that check for null, that fail now since a mixed[] can be null.
I deleted them since it is no longer a typing error. But I'm unsure if that is the right call to make.

@sindresorhus
Copy link
Member

Can you fix the merge conflict?

@sindresorhus
Copy link
Member

However there are two flow tests that check for null, that fail now since a mixed[] can be null.
I deleted them since it is no longer a typing error. But I'm unsure if that is the right call to make.

All good 👍

@sindresorhus
Copy link
Member

Looks like you resolved the merge conflict incorrectly. Look at the diff.

@vanbujm
Copy link
Contributor Author

vanbujm commented Dec 28, 2018

@sindresorhus Okay, I think it is sane now. I rebased when I did the first merge and thats what made everything crazy. Ended up just going back to pre-merge, merging (correctly) this time, then force pushing my fork. There was probably more elegant git wizardry that could have been done but ¯\(ツ)

@sindresorhus sindresorhus changed the title Changed tagged template literal argument type to unknown Change tagged template literal argument type to accept unknown instead of just string Dec 28, 2018
@sindresorhus sindresorhus merged commit 7f6e563 into chalk:master Dec 28, 2018
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.

None yet

2 participants