Repository navigation
Allow partial application of input - #762
Conversation
🦋 Changeset detectedLatest commit: a57d858 The changes in this PR will be included in the next version bump. This PR includes changesets to release 9 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 |
6c59968 to
bf13511
Compare
|
(obviously will add tests if the general principle is approved) |
|
Yup I’m on board with this! I think it’s the user’s choice to want to only act on a subset of a resolver, the resolver design isn’t dependent on it being irreducible at every level. To me this is the same as simply deleting part of the document anyway, which should be the author’s choice to do so. |
|
Added a couple of simple tests. I've added my go-to blue-orange/light-dark resolver for testing more intricate usage. |
|
One limitation of this is that it still wont work if you have an However, solving this would probably require something a bit more sophisticated implementation wise, whereby the root document is created from merging all sources, but recording in which source each token was declared. Then, at token resolution time, only resolving tokens that come from sources on the allow list. |
| }); | ||
|
|
||
| describe('partial application', () => { | ||
| let resolver: Resolver; |
There was a problem hiding this comment.
As a general rule we should try to not share state between tests. It’s a really hard mistake to undo when tests now have to execute in a certain order otherwise the entire suite breaks. Even if this is “safe” as-written, it only takes one modification or two here (likely by another author that doesn’t understand) the suite is poisoned with state contamination.
Instead, let’s move this to a pure loadResolver() helper that’s called within each test. Then each test sets up what it needs, and doesn’t affect other tests, and we still are DRY without sharing state.
There was a problem hiding this comment.
I just miss fixtures from pytest...will update :)
There was a problem hiding this comment.
Ah I didn’t realize it came from Python! Yeah I’ve just spent almost 10 years untangling leaky state from large test suites 🙃. Even in a smaller project like this library, it still turns into a real timesink when it breaks, unfortunately (especially because JS test runners like Vitest operate using a lot more magic than one might think)
There was a problem hiding this comment.
/wildly off topic
pytest has a full dependency injection framework, so you never need to use an explicit beforeEach but instead you'd do something like:
@fixture
def resolver():
# parse your resolver here
return resolver
@test
def test_resolver(resolver):
# resolver fixture injected by name
assert resolverhappy days. The fixtures can have different scopes to share resources (and save processing time), cleanups that will run even if tests fail. Oh, and it uses native python assertions with some horrific byteweaving to add in rich failure messages.
e779eed to
c4b8459
Compare
That’s fine for now IMO. The two flavors of that—DTCG alias and JSON Schema |
| export type ResolverApplicationOptions = { | ||
| sets?: string[]; | ||
| modifiers?: string[]; | ||
| resolveAliases?: boolean; |
There was a problem hiding this comment.
Can we add this to the documentation for the JS API? I want to make sure I’m understanding when I want this to be true or false. A short section & example would be great if you can provide one!
There was a problem hiding this comment.
@drwpow done - did some drive-by polishing of some adjacent dusty bits, but would welcome a redraft my redraft.
drwpow
left a comment
drwpow
left a comment
There was a problem hiding this comment.
Amazing, thank you for adding! Good with this merging, would you mind adding a minor changeset (see comment) so we can release it? Thanks!
| const r = createResolver(resolver); | ||
|
|
||
| for (const input of r.inputPermutations) { | ||
| for (const input of r.listPermutations() ?? [{}]) { |
| } | ||
| ``` | ||
|
|
||
| It’s up to you to specify all the permutations desired. `.getAllInputPermutations()` will return an array of all possible inputs for the specified modifiers. |
There was a problem hiding this comment.
🤦 Had quite a few old references to some early things that got renamed, I see. Thanks again
Changes
resolveAliasesoption toResolver#applyAPIWhy?
This now allows you to do:
and the result will contain only tokens declared in
a-set. The same is possible for modifiers.This is of great benefit when writing plugins that need to output only the tokens declared in a given set/modifier (e.g., in the case of a css class per context per modifier).