chore: Stable hashes for border tokens - #4718
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4718 +/- ##
=======================================
Coverage 97.59% 97.59%
=======================================
Files 950 950
Lines 30662 30662
Branches 11240 11240
=======================================
Hits 29926 29926
Misses 689 689
Partials 47 47 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
5eb621e to
47e8275
Compare
| counters.unset = (counters.unset ?? 0) + 1; | ||
| } | ||
| } | ||
| console.log(`Design token versions: ${JSON.stringify(counters)}`); |
There was a problem hiding this comment.
Why do we need this log? I am thinking we could make it separate function. because the it seems lke getTokenVersions is doing two things (logging the count of token version, and returning the token versions).
There was a problem hiding this comment.
Logging is just the debugging - it helps to understand what portion of the tokens is already covered with versions, that's it.
| const versionGroups = [{ pattern: /^border-/, version: DEFAULT_TOKEN_VERSION }]; | ||
|
|
||
| // Builds the token -> version allowlist from the full token -> cssName map. | ||
| function getTokenVersions(variablesMap, groups = versionGroups) { |
There was a problem hiding this comment.
Consider separating this function into three functions: getTokenVersions, findVersion, and logVersionCount (if this is needed, if not let's remove the counter, and log). That way we can also test the findVersion separately.
There was a problem hiding this comment.
Why to test findVersion separately? The entire function and the pattern we use for border tokens are already covered with tests. The logging does not need to be tested - that is just for debugging.
|
|
||
| // Groups map a token-name pattern (matched against the token's CSS variable name) to a version. | ||
| // Tokens matching no group stay version-less and keep the legacy value-based hashes. | ||
| const versionGroups = [{ pattern: /^border-/, version: DEFAULT_TOKEN_VERSION }]; |
There was a problem hiding this comment.
From the comments and implementation it's not clear that the order in our version group matters, for the getTokenVersions (which is a very important thing). Can we make this more robust? so that our tests fail when the order changes? or we don't need the method getTokenVersions to be dependent on the order of the versionGroups.
Do we also need a way to detect a conflicting groups?
There was a problem hiding this comment.
2 participants
Description
A follow-up for cloudscape-design/theming-core#190
How has this been tested?
Review checklist
The following items are to be evaluated by the author(s) and the reviewer(s).
Correctness
CONTRIBUTING.md.CONTRIBUTING.md.Security
checkSafeUrlfunction.Testing
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.