Skip to content

Conversation

@Flaque
Copy link

@Flaque Flaque commented Jul 25, 2025

Overview

This PR adds support for the rest of CEL.

We @ sfcompute.com are interested in using CEL for ourselves, but would like a reasonable typescript implementation to exist.

So this PR hooks up the conformance tests from the CEL spec into a test framework, and then iteratively makes all of them pass.

It adds functionally every missing feature and resolves a lot of the edge cases from the existing implementation.

Apologies for the large PR!

Checklist before requesting a review

  • I have performed a self-review of my code
  • I have added tests (if possible)
  • I have updated the readme (if necessary)
  • I have updated the demo (if necessary)

Flaque added 12 commits July 1, 2025 11:58
- Merged optional chaining and quoted identifier support with existing collection macros
- Combined CST definitions from both branches
- Integrated parser changes for better identifier handling
- Resolved visitor conflicts while preserving all functionality
@ChromeGG
Copy link
Owner

Whoa, what a wonderful PR!
I'll review & run it tomorrow morning (in 10h). Please fix the conflicts with main branch
Amazing job :)

Flaque and others added 5 commits July 27, 2025 17:52
- Merged optional chaining and quoted identifier support
- Kept collection macros functionality
- Resolved conflicts in cst-definitions.d.ts, parser.ts, and visitor.ts
@ChromeGG
Copy link
Owner

ChromeGG commented Jul 28, 2025

Please take care of these issues so I can proceed with the review:

1. Eslint Issues

There are 575 eslint problems (538 errors, 37 warnings). While not all of them are significant, a large portion is, such as:

const result: any = {} 
// can be
const result: Record<string, unknown> = {}

// no IF statements in the tests, etc.
// no unused variables

2. Code Formatting

Please run prettier on all the files to ensure they align with the repository conventions.

3. Comments

Please address the comments I left.


Also the visitor.ts becomes a monster (over 6k LOC). IDK if it's an issue but it can be hard to maintain. But I have no idea how to refactor it 😅 But it's just nice-to-have fix, not a blocker

@SksOp
Copy link

SksOp commented Sep 9, 2025

Hey i just saw this PR, I was about to open a few issues without knowing this exists. @Flaque are you still working on this? I also need this in my project and would like to know the progress and if you need some help.

@ChromeGG
Copy link
Owner

@SksOp I think there is no progress on it. You can fix the issue I listed above. Happy to merge it once it's resolved

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants