Skip to content
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

fix: fixed conditional selection bug (empty base schema) #75

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

iduuck
Copy link
Collaborator

@iduuck iduuck commented Feb 24, 2023

fixes #25

also touches #68

@iduuck
Copy link
Collaborator Author

iduuck commented Feb 24, 2023

Please review, @gksander.


Sorry for ping, but couldn't request a review, since I am no maintainer.


invariant(data);
expect(data[0]).toEqual({ name: "Bulbasaur", attack: 49 });
expect(data[1]).toEqual({});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is the thing that is getting me hung up. If there's an empty base selection, and our type looks something like ({ name: "Charmander", ... } | { name: "Bulbasaur", ... })[], then having an empty object is a "wrong" according to our type.

But in groq, I believe you can have an empty projection and it'll just return an empty object. So for example, this query:

*[_type == 'pokemon'][0..3]{...select(name == 'Charmander' => { name, "hp": base.HP }, name == 'Bulbasaur' => { name, "attack": base.Attack })}

does indeed pass the tests you have here, including the empty-object one – since that projection will just select nothing on everything but Charmander and Bulbasaur.

Still trying to wrap my head around this, but I think limiting our types on this might end up causing a mismatch with how groq behaves – which would be confusing.

Any thoughts on this? It's not yet clear to me how to proceed on this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Jep, that's true. Also don't know, how to proceed either. Then, I think, the PR can be closed, and it's up to the user, to check, whether the empty object is matching, or not.

Though, on the other side, if there is an empty object in the response, I don't think there is a big case for any user to show an empty tile/card/list-item/whatever, so my opinion on this would be to just add a third argument to the conditionals, and just set a default option.

I am thinking of an option named includeEmptyObject, which is then conditionally giving the user at least the type. But then, we would need to filter the empty object on our side.

@maxyinger
Copy link
Contributor

So we're working on an idea with a new select() method that was part of the 0.10.0 release. You can check out the details in this PR here, but it's basically for the case where you have an empty base selection, and your sanity data falls into one of a few shapes.

To omit the empty object (or null depending on which context you use it), you pass it a default case. So here it would look like:

const { data, query } = await runPokemonQuery(
      q("*")
        .filter("_type == 'pokemon'")
        .slice(0, 3)
        .select(
          {
            "name == 'Charmander'": {
              name: q.literal("Charmander"),
              hp: ["base.HP", q.number()],
            },
            default: {
              name: q.string(),
              attack: ["base.Attack", q.number()],
            },
          }
        )
    );

or if you wanted specifically Bulbasaur & Charmander, you could add it to the filter, ie:

const { data, query } = await runPokemonQuery(
      q("*")
        .filter("_type == 'pokemon' && (name == 'Charmander' || name == 'Bulbasaur')")
        .select(
          {
            "name == 'Charmander'": {
              name: q.literal("Charmander"),
              hp: ["base.HP", q.number()],
            },
            default: {
              name: q.literal("Bulbasaur"),
              attack: ["base.Attack", q.number()],
            },
          }
        )
    );

@scottrippey
Copy link
Member

In the groq-builder branch, we've addressed this issue by automatically including the empty object in the result type, but there's an option called isExhaustive that won't include the empty object type.

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.

Empty base selection for grab should not create an empty object schema for validation
4 participants