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.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Onyx derived values #56891
base: main
Are you sure you want to change the base?
Onyx derived values #56891
Changes from 26 commits
f57151d
0733f7d
f05f33c
8a576be
502b3c6
4c343ef
fe56c68
f505c0b
cb92cfc
4817676
05e17ff
7de6fc5
1cb4cbe
d3e7010
1ab4d35
a922c34
c045ef2
867f5e4
b96d4a6
0d08483
27ece8d
25bdbf4
aed0d95
446fead
2a76669
0a91c78
d226365
c040ec0
140c793
b9f1d50
88ba460
9e57693
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These changes were only necessary because I upgraded
type-fest
and a bug fix in that repo exposed existing issues with the types in this code.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure we can't use satisfies? 🤔 I feel like it works well for me:
Diff
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, but you'd actually want it to, rather than being an
OnyxDerivedValueConfig<any, any>
be andOnyxDerivedValueConfig
for that specific key.Plus, I like how this provides clearer error messages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okaaay, I believe we can achieve it with a mapped type! Recorder a video to showcase the DX:
Screen.Recording.2025-02-24.at.11.15.31.mov
Diff as always
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@roryabraham Let me know if this is what you wanted to achieve 😄 With this approach ONYXKEYS' mapping is the source of truth while keeping type safety in ONYX_DERIVED_VALUES per key, please test it yourself!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great work! I just pushed a variant of this that I believe is fully correct. The main difference between my version and yours is that mine does not coerce the return type of
compute
to be anOnyxEntry
.The error messages suck, in particular when you have a collection in the dependencies. But that seems like a common problem in TS in general. I tried adding some assertions to provide better error messages, but didn't get it working and don't want to sink too much time into it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
API could be improved a little, so that array isn't needed:
Full diff in case you're interested:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At the same time I'm thinking if it'd be clear enough without a tuple? I think my only concern is that swapping dependencies in both these cases can break the whole pipeline.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But the same would happen with or without an array, right? In the end you have TS type checking to make sure you use it correctly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like array-like syntax for the reason that its kind of norm for many modules. e.g.
Promise.all
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just suggesting that it is possible! I'm good with both approaches 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we added the current value as a second arg to
compute
, I think I'm going to leave the first as a tuple. That way it's not like you haven
args and the last one happens to be the current value