-
Notifications
You must be signed in to change notification settings - Fork 24
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
feat(evaluator): simplified synchronous evaluator #273
base: main
Are you sure you want to change the base?
Conversation
New dependencies detected. Learn more about Socket for GitHub ↗︎
|
} | ||
Object.defineProperty(acc, attribute.name, { | ||
enumerable: true, | ||
get: () => evaluate({...context, node: attribute.value}), |
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.
lazy evaluation based on property access. what a great idea @judofyr
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.
Nice! I like this!
interface EvaluateOpCallOptions extends Context { | ||
node: OpCallNode | ||
} |
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 don't quite see the need for this. Why not use multiple parameters? Isn't the current restructuring just adding lots of unnecessary allocations anyway?
The main reason here was to be as close to the spec as possible (e.g. https://spec.groq.dev/draft/#sec-Scope) which has some educational value, but it's not a very big deal. |
I think this makes a lot of sense for a groq-js v2. We can still in a minor release introduce an
I think we might need to keep our own
Let's also be way more precise (both in the API and preferably in the types) around what types we actually handle in the evaluator. We shouldn't need to use |
This is still a work in progress (and is fairly adventurous) but I wanted to share this direction for feedback before I continue down this path.
Value
wrappers didn't seem to be necessary in this model so I tried removing it. It seems to be pretty doable but handling dates requires checking a string to see if it's a date first and that may be problematic. At the current state of this PR, the date handling is close but still wrong.@portabletext/toolkit
to get the portable texttext
value. I haven't checked if the output is the same but this approach is super appealing if there isn't a difference in output.Scope
to an array of values which somewhat works but doesn't have a concept ofroot
anddataset
How does this look so far? This PR isn't ready for an in-depth review but I'd love thoughts on this direction. It's a large change but the resulting code is greatly simplified. Is it worth it?
Let me know what you think. Is this a good candidate for a groq-js v2? Or should this be just tacked on to the existing code? Or do we want to support both async and sync evaluation with an approach similar to #272 ?