feat(resolver): Add extra methods for token set operations - #830
feat(resolver): Add extra methods for token set operations#830AngelRionCervi wants to merge 1 commit into
Conversation
🦋 Changeset detectedLatest commit: 2a6da5b The changes in this PR will be included in the next version bump. This PR includes changesets to release 13 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 |
drwpow
left a comment
There was a problem hiding this comment.
This is awesome! 💯 I totally see the value in this addition, and am in favor of adding.
Before I approve though, would love your thoughts on the API change, and otherwise would like to see the new tests split out into their own fixture to make sure all the changes are 100% additive and backwards-compatible. But overall love the direction and thought here.
| * | ||
| * @default 'both' | ||
| */ | ||
| tokenOrigin?: 'alias' | 'primitive' | 'both'; |
There was a problem hiding this comment.
Suggestion: I love this ability to filter tokens based on whether they are the source of a value or not! But could we adjust the API a little bit, and reach the same end result, but allow for more usecases?
/**
* Filter tokens based on specific criteria
*/
filter?: (token: TokenNormalized) => boolean;So for example, for 'primitive', you could do:
filter: (token) => !token.aliasOf?.lengthAnd for 'alias':
filter: (token) => token.aliasOf?.length > 0These should fit y’alls usecases. But I’m also thinking of other design systems I’ve come across where they really use namespacing in a significant way, and want to do something like:
- filter: (token) => !token.aliasOf?.length
+ filter: (token) => !token.aliasOf?.length && !token.id.startsWith('deprecated.')Open to thoughts! My only hesitance with this API as-written is not the practicality of it, it’s more that the idea of “primitive” tokens isn’t a concept or verbiage that exists elsewhere in Terrazzo. But the idea of a configurable function is a pretty common pattern, that we can provide some “recipes” like this that equate to the same things.
There was a problem hiding this comment.
Ah yes ! I agree it's better, I will make that change 👍
| }, | ||
| }; | ||
|
|
||
| return addResolverExtras(resolver); |
There was a problem hiding this comment.
2 things:
- we could just add to the object here, rather than creating a duplicate object in memory inside
addResolverExtras - I don’t mind everything on
.extrasjust being part of the core API here 🙂.resolver.intersectionandresolver.semanticDifferenceare a lot cleaner
| "sources": [{ "$ref": "primitives.json" }] | ||
| }, | ||
| "invariants": { | ||
| "sources": [{ "$ref": "invariants.json" }] |
There was a problem hiding this comment.
Change: rather than change this test fixture, can we just copy this and fork it into a new one? I can see that a lot of the changes aren’t a huge departure from the original, but there are lots of little changes here I want to make sure the original tests don’t have any hidden mini-regressions here.
Even if there’s some bit of test duplication, that’s OK 🙂 I don’t mind cleaning up later
|
Thanks for the review and for taking the time to explain your points ! I pushed modifications based on your comments but I also converted the PR to a draft to add the methods and a couple of examples to the doc if that's ok |
688dcb4 to
e82c80d
Compare
e82c80d to
2a6da5b
Compare
|
Added the doc edit: |
This PR adds utility functions to the resolver to manipulate and filter resolved tokens.
Motivations
My team and I have trouble converting our codebase to use the resolver, one of the reasons is that we have to do some amount of filtering of the resolved tokens ourselves, which we could avoid when using legacy modes.
For example, for optimization reasons, we want to be able to filter out all the tokens common to every modifiers (meaning their value don't change), in order to create a
common.cssfile, and then have tokens which values are exclusive to a modifier context in their own{modifier}-{context}.cssfiles. This way, tokens with the same values across all modifiers are stored in one file, and we avoid having duplicated tokens in other files.Changes
This PR adds 2 methods in
resolver.extra:intersection(options): returns all tokens that don't change value across modifierssymmetricDifference(options): returns all tokens on which the value changes across modifiersHow to Review
Check the added tests in
packages/parser/test/resolver.test.tsand verify that they passThis is very much an idea open for suggestions 👍