fix(utils): keep capitalize from lowercasing the rest of the string - #876
Open
pucedoteth wants to merge 1 commit into
Open
fix(utils): keep capitalize from lowercasing the rest of the string#876pucedoteth wants to merge 1 commit into
pucedoteth wants to merge 1 commit into
Conversation
capitalize is declared as returning Capitalize<T>, which uppercases only the
first character and leaves the remainder untouched. The implementation
lowercased everything after it:
str.charAt(0).toUpperCase() + str.toLowerCase().slice(1)
so the value contradicted its own type:
capitalize('USD') // typed 'USD', returns 'Usd'
capitalize('helloWorld') // typed 'HelloWorld', returns 'Helloworld'
capitalize('a11yLabel') // typed 'A11yLabel', returns 'A11ylabel'
Because the declared type is a literal, TypeScript will accept the result
wherever the original literal is required, while the runtime value differs.
Drop the toLowerCase() call. Pairing with wordCase is unaffected: decamelize
already lowercases its output, so capitalize(wordCase('helloWorld')) is
'Hello world' either way. The output only changes for inputs that carry
uppercase characters past the first position, which are exactly the inputs
whose current result violates Capitalize<T>.
packages/utils had no tests. Added string.test.ts covering capitalize, the
case helpers and the css var helpers.
Collaborator
🟡 Heimdall Review Status
🟡
|
| Code Owner | Status | Calculation | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
| ui-systems-eng-team |
🟡
0/1
|
Denominator calculation
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
capitalizeis declared as returningCapitalize<T>, which uppercases only the first character and leaves the remainder untouched. The implementation lowercases everything after it:So the value contradicts its own type:
capitalize('USD')'USD''Usd'capitalize('ETH')'ETH''Eth'capitalize('helloWorld')'HelloWorld''Helloworld'capitalize('a11yLabel')'A11yLabel''A11ylabel'Because the declared type is a literal, TypeScript accepts the result wherever the original literal is required, while the runtime value differs. The
as Capitalize<T>assertion is what lets that through.Why this direction
capitalizecould equally have been meant as lodash-style ("first upper, rest lower"), in which case the type would be the thing to change. Two reasons for fixing the value instead:wordCaseisdecamelize(str, { separator: ' ' }), whose output is already lowercase, socapitalize(wordCase('helloWorld'))is'Hello world'before and after.The output only changes for inputs carrying uppercase characters past the first position, which are exactly the inputs whose current result violates
Capitalize<T>.If you would rather keep the lowercasing and relax the type instead, that is a one-line change in the other direction and I am happy to switch.
Tests
packages/utilshad no test files. Addedpackages/utils/src/string.test.tscoveringcapitalize(including the empty string, an already-capitalized input, and thewordCasepairing), the case helpers, and the css var helpers.Verification, and what I could not run
I could not run
yarn nx run utils:testhere. The repo is ~1 GB and I worked from a partial clone, so the working tree is not complete enough to install and run the monorepo toolchain. Flagging that rather than implying the suite passed.What I did do: executed every assertion in the new test file against the real
humpsbuild, sincecamelCase/kebabCase/snakeCase/wordCasedelegate to it. All 13 hold, including thecapitalize(wordCase(...))case. Thecapitalizechange itself is one expression and is covered by the table above.