# Bug 3104: `or` API improvements

- Status: closed
- Project: Zero
- Creator: @aboodman
- Assignee: @arv
- Labels: alpha-must
- Created: 2024-10-31T01:08:14Z
- Modified: 2024-11-15T14:08:10Z
- URL: https://bugs.rocicorp.dev/p/zero/issue/3104

## Description

I am retargeting this bug to be specifically about Dax' complaints about the `or` API.

Context:

- https://discord.com/channels/830183651022471199/1288232858795769917/1301048822860091393
- https://rocicorp.slack.com/archives/C013XFG80JC/p1730321354457489
- https://discord.com/channels/830183651022471199/1288232858795769917/1303071060073381920

## Comments (8)

### @aboodman — 2024-10-31T01:28:38Z

I suspect the right thing is to have a `string`/`any` overload for the `cmp` function, but am open to other solutions like a builder.

### @aboodman — 2024-10-31T01:29:02Z

I like changing `cmp` for the usual reason I like anything - introduces no new primitive, reuses existing tools.

### @arv — 2024-10-31T13:34:25Z

The benefit of a builder is that we can flow the types into the closure. We've previously had problems with typescript not being able to infer the types inside the closure.

I don't think a builder will help with the type `string` being passed into a function wanting the column name but like Aaron said we can probably also allow string which leads to the output return type being `Row`.

### @arv — 2024-11-01T14:41:13Z

One of the problems with allowing `string` is that typos are not caught.

```ts
// schema has userID column
q.issue.where('userId', 'holden')
```

I almost think it is better to do something along the lines of:

```ts
q.issue.where(expr as keyof Issue, value)
```

and leave the strict types in place

Reactions: 👍️ ×1 (tantaman)

### @aboodman — 2024-11-05T09:06:05Z

OK we all keep getting confused and thinking:

```ts
q.issue.where(expr as keyof Issue, value)
```

Is a problem to be solved here. It is not. In the original bug report the field portion was static (always `'category'`). The only problem to be solved in original bug report is types getting lost by pulling `or` and `cmp` out of the context of `where`.

After thinking about this some I do think what we should do is copy the kysley approach / use a callback:

```ts
let q = z.issue.where({cmp, or, and, not}=> or(
    filters.map(f => cmp('category', f))
));
```

Few more things:

1.  For user convenience you should be able to pass `undefined` to a term of `or` and have it filtered out: https://discord.com/channels/830183651022471199/1288232858795769917/1303095261048082592

2. In the future (not this bug), we can easily extend this to the following imperative form if desired:

```ts
let q = z.issue;
const cmps: Array<typeof q.cmp> = [];
for (const f of filters) {
  cmps.push(q.cmp('category', f));
}
q = q.or(cmps);
```

### @arv — 2024-11-12T12:38:07Z

Re undefined terms:

This could lead to a call with 0 terms.
- For OR, the logical behavior of that is that it NEVER passes.
- For AND, the logical behavior of that is that it ALWAYS passes.

### @tantaman — 2024-11-12T15:24:06Z

~~Arv's proposal looks correct to me after working through some expressions in both DNF and CNF to make sure they get the same result.~~

There are some inconsistencies: https://github.com/rocicorp/mono/pull/2979#discussion_r1838486763

I think: empty `OR` always passes, empty `AND` always passes, `undefined` in an `AND` or `OR` chain has no impact.

### @arv — 2024-11-15T11:51:11Z

The main work is done.

I wanted to look at this part before marking as done:

```ts
let q = z.issue;
const cmps: Array<typeof q.cmp> = [];
for (const f of filters) {
  cmps.push(q.cmp('category', f));
}
q = q.or(cmps);
```