CSS Plugin: Handle Whitespace in Token Ids #131
Conversation
🦋 Changeset detectedLatest commit: 7a7a1d1 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
| @@ -76,12 +76,14 @@ export default { | |||
| embedFiles: false, | |||
| /** (optional) transform specific token values */ | |||
| transform: () => null, | |||
| /** (optional) add custom namespace to CSS vars */ | |||
| /** (deprecated, use generateName instead) add custom namespace to CSS vars */ | |||
There was a problem hiding this comment.
Thanks for updating the JSDoc hints! I can make this change after merge, but I’ll put a /** @deprecated tag on it and that shows up in the IDE
| }), | ||
| ], | ||
| }; | ||
| ``` |
There was a problem hiding this comment.
💯 Awesome docs addition, thank you!
| expect(defaultNameGenerator('path.to.token', 'scope-')).toBe('scope-path-to-token'); | ||
| }); | ||
| }); | ||
| }); |
| variableId: string, | ||
| token?: ParsedToken, | ||
| ): string => { | ||
| const name = customNameGenerator ? customNameGenerator(variableId, token) : defaultNameGenerator(variableId, prefix); |
There was a problem hiding this comment.
I like your implementation, but I will make one tiny change here in a patch release: we call a user’s customNameGenerator() on every token, and they’re allowed to return undefined or null if they’d like to defer naming back to us.
This is common plugin behavior (example: Rollup) and is a super minor detail that doesn’t affect the overall code. Just a tiny logical change that rather than checking a user passed the function, instead check the return value of the function and fall back to defaultNameGenerator() if customNameGenerator() executed but returned a falsy value.
There was a problem hiding this comment.
The breakage was really impossible to test for; it had to do with mismatching versions of plugin-css and plugin-sass. And since we didn’t indicate this as a breaking change, we shouldn’t require both be upgraded (I didn’t realize it either). Not really a way to test for this (maintaining old versions in the test suite just to test compatibility is a path to madness).
So note I’m not changing any code you wrote or how this function works; just going to change some internal function signatures and release a patch for plugin-css and plugin-sass so both can work correctly with mismatched versions (which, ironically, updating both to the patched version would fix the problem anyway, but it at least signals to consumers that a problem was identified and fixed).
Again, no one’s fault, not a big deal, and just sort of a weird, unpredictable, hard-to-test-for scenario. But wanted to leave a note.
| /** reference an existing CSS var */ | ||
| export function varRef(id: string, options?: {prefix?: string; suffix?: string; fallbacks?: string[]; mode?: string}): string { | ||
| export function varRef(id: string, tokens: ParsedToken[], generateName: ReturnType<typeof makeNameGenerator>, suffix?: string): string { |
There was a problem hiding this comment.
Also a note: will have to revert some of the changes to the varRef API as it’s used outside this plugin (may not be best practice, but it is what it is).
Mainly will revert the ordered params and revert back to a single object param for options (which is preferred in general, because then you can add/remove options easily, and handle backwards compatibility, with minimal effort)
| export function defaultTransformer(token: ParsedToken, {colorFormat = 'hex', mode, prefix}: {colorFormat: Options['colorFormat']; mode?: string; prefix?: string}): string | number | Record<string, string> { | ||
| export function defaultTransformer( | ||
| token: ParsedToken, | ||
| tokens: ParsedToken[], |
There was a problem hiding this comment.
Also here, too—didn’t notice this change to the defaultTransformer() function signature that was a breaking change for this plugin (because it taps into transform()). Again, there were no tests to catch this, so that’s a fault of the test suite! Just leaving notes.
Changes
This PR fixes #123 by removing leading and trailing white space from token and token group names and camelCasing those with white space in the middle of their names.
It also adds a new plugin option called
generateNamethat allows the consumer to customize that behavior (and everything else about how a name is generated). This makes theprefixoption redundant, so it has been deprecated.How to Review
I broke the functions in the
util.tsfile into their own files in autilsfolder for improved readability and test-ability, which made it a lot easier to work on the variable name conversion refactor in isolation. I also added tests for the bug fix and the new option.Observe that the unit and integration tests added for handling whitespace in token and group names pass.
The documentation added can be seen at the bottom of the options section of the plugin's page.