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

Allow assigning subtypes in queries #606

Draft
wants to merge 21 commits into
base: master
Choose a base branch
from

Conversation

scotttrinh
Copy link
Collaborator

Closes #592

This adds a new property to the type that contains all of the subtypes of a given type, so that when testing assignment, instead of comparing objects structurally we compare just that the current type is a supertype of the assigning type by comparing the listed subtypes of the assigning object.

In other words, if a type chain is A -> B -> C -> D, then type A will reference all 4 types, and attempts to assign C (which contains C and D) will succeed.

Note that this will break if anyone was relying on structural typing before since if two unrelated types were being used before but shared the same structure, the old assignment type was typechecking. I think it's probably the case that it would've been a runtime error, so I'm not concerned that there are valid use cases, but it's worth having a think about it before running with this.

@scotttrinh scotttrinh requested a review from jaclarke May 17, 2023 14:37
@Arrow7000
Copy link

I can't really wrap my head around the type generation code, but this does seem to fix my use case!

image

@Arrow7000
Copy link

There seems to be something wrong with the default exports though. I'm getting this, but the one at the bottom looks very off.

image

I'm guessing it's from this part of my schema? Looks like it was mis-parsed though.

Code_njZX8SUFej

@Arrow7000
Copy link

I don't really understand how TypeSet works, but it seems wrong that it contains Cardinality.AtMostOne | Cardinality.One | Cardinality.Empty when this field only has a cardinality of exactly Cardinality.One?

image

@scotttrinh
Copy link
Collaborator Author

@Arrow7000 Thanks for digging in here!

There seems to be something wrong with the default exports though. I'm getting this, but the one at the bottom looks very off.

Is this a regression in this branch compared to master? We recently released a change to allow more unions (#279) which I know you got a chance to see as well, but none of this code should've broken any of that union parsing.

I don't really understand how TypeSet works, but it seems wrong that it contains Cardinality.AtMostOne | Cardinality.One | Cardinality.Empty when this field only has a cardinality of exactly Cardinality.One?

I'll take a look at this, but it's likely due to the subtype inclusion being too permissive. Can you share what $StrategyGamer and the other related types look like in your generated code?

@scotttrinh
Copy link
Collaborator Author

scotttrinh commented May 18, 2023

Going to set this as a draft for now while I add a bunch of tests in a separate set of PRs that branch from this. But, if anyone is already reviewing this definitely feel free to continue and report back here.

@scotttrinh scotttrinh marked this pull request as draft May 18, 2023 01:01
Conflicts:
	packages/generate/test/globals.test.ts
	packages/generate/test/group.test.ts
	packages/generate/test/insert.test.ts
	packages/generate/test/literals.test.ts
	packages/generate/test/params.test.ts
	packages/generate/test/queries.test.ts
@Arrow7000
Copy link

Hey @scotttrinh, just checking in on this PR. I don't know if this was ready to merge or not, was it waiting on anything else?

@scotttrinh
Copy link
Collaborator Author

@Arrow7000

Yeah, it's mostly waiting on me to have the time to work out the implications "downstream" from implementing what is essentially nominal typing within the TypeScript client. No ETA on this, but it's not ready to merge yet. We're hard at work on finish up some big stuff for EdgeDB 4.0 coming soon, but after that I'm going to circle back to a bunch of WIP stuff on the JS client side.

I appreciate your patience!

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.

Making overloaded link required and subtyped in "grandchild" of inherited type breaks query builder
2 participants