Polymorphic queries -> discriminated union types #1090

CarsonF · 2024-08-31T21:19:20Z

I revived #508

Fixes #454

Probably a breaking change since the types are getting stricter.

…ymorphic queries as discriminated union

…y unions)

CarsonF · 2024-09-01T20:40:03Z

I restored some of the old functionality, and gated the stricter types behind an opt-in via declaration merging. Just looking for ways to get this out without a major. But open to reverting it or other suggestions.

packages/generate/src/edgeql-js/generateObjectTypes.ts

… `__type__.name` is the proper path.

CarsonF · 2024-09-04T13:47:25Z

packages/generate/src/syntax/path.ts

+      ? ScalarType<
+          "std::str",
+          TypesystemOptions["future"]["strictTypeNames"] extends true
+            ? Root[typeof typenameSymbol]
+            : string,
+          Root[typeof typenameSymbol]
+        >


I'm not sure I understand the difference between Scalar's TsType & TsArgType. I was thinking that they were the output / input types respectively.

I don't understand why the input type would be called out here explicitly and set to the literal string - where/how would that be used be as an input?

I figured the output type remained as a string for BC reasons. So I created another future option to make it strict.
This is for this btw:

scope => ({ t: scope.__type__.name })

But then I was thinking about this case

scope => ({ __type__: { name: true } })

Which had already made the output type a strict string.

So I thought the other case was a miss, but there is some explicit code here.

I guess all that to say I could just do this:

Suggested change

? ScalarType<

"std::str",

TypesystemOptions["future"]["strictTypeNames"] extends true

? Root[typeof typenameSymbol]

: string,

Root[typeof typenameSymbol]

>

? ScalarType<"std::str", Root[typeof typenameSymbol]>

@scotttrinh thoughts here? I don't think we need a flag for this & just use my suggestion

Good question. I'm not sure, but I'm fine with going with the suggested change for now and looking at what the fallout is.

integration-tests/lts/select.test.ts

…a type intersection expr

CarsonF · 2024-09-05T13:27:27Z

packages/generate/src/syntax/toEdgeQL.ts

+  if (hasPolyEl && !seen.has("__typename")) {
+    addLine("__typename := .__type__.name");
+  }


The new functionality implies a __typename field when needed. If the user specifies this and it doesn't match what is assumed then the types will be wrong. We could check the __typename value here if it is already present (from a user) and confirm it is .__type__.name or throw.
I couldn't figure out a clean way to do this because the actual generated EdgeQL explicitly references the scope, which I couldn't figure out how to access.

single __typename := __scope_1_defaultMovie.__type__.name,

ok, we can leave that as a follow up investigation, thanks for the details!

scotttrinh

Very solid 👏 Thanks for the huge effort here. Love that we now have some feature-flagging/opt-in machinery set up in the typesystem, too, that seems generally very useful.

scotttrinh · 2024-09-19T16:11:36Z

packages/generate/src/syntax/toEdgeQL.ts

+  if (hasPolyEl && !seen.has("__typename")) {
+    addLine("__typename := .__type__.name");
+  }


ok, we can leave that as a follow up investigation, thanks for the details!

scotttrinh · 2024-09-19T16:16:47Z

One thing I want to think about is maybe having some tests with the legacy behavior just to catch any regressions we make between now and when we switch the new behavior to the default in a future major release. I can add those.

scotttrinh · 2024-09-19T17:24:48Z

packages/generate/src/syntax/toEdgeQL.ts

@@ -1299,6 +1301,7 @@ function shapeToEdgeQL(
    if (val.__kind__ === ExpressionKind.PolyShapeElement) {
      polyType = val.__polyType__;
      val = val.__shapeElement__;
+      hasPolyEl = true;


Ah, we don't actually have a runtime representation of the type-level "opt in" for this behavior, so this ends up still being an arguably breaking change since we will be returning more. Definitely arguable as to whether or not returning strictly more data is breaking, but anyone who currently has tests for the existing behavior will get failures.

I wonder if we can make a runtime configuration, and actually infer the type-level config from that runtime config?

I'd probably recommend a generator config flag in that case.

yeah, agreed. we can even have it generate/copy a different typesystem.ts file to avoid the overhead of being conditional in the types.

I probably wouldn't say that adding an additional field is a breaking change. User tests may not break. Only if they are asserting exactly or using snapshots.

But I can see how it's not black/white. IMO none of this stuff is. kinda like changes in TS releases lol

User tests may not break. Only if they are asserting exactly or using snapshots.

I think this is pretty common: make a query (or use a data layer more likely) and assert that the returned data is precisely these objects. I agree in general that if you are introducing new properties/data in your own application, maybe you don't consider that a breaking change, but this might warrant a little more care to avoid tripping up people when upgrading point releases.

Are you looking for a CLI flag for this as well?

Yeah, I think we should add it, like --enable-polymorphism-as-discriminated-unions as silly as that sounds. This is non-trivial, but I think it's worth it to avoid going with a major version bump here. I'm honestly open to the major version bump, but that will absolutely delay getting this released.

If you think you have the bandwidth to add the CLI flag, let me know. Otherwise, I'll stick it in the queue and try to get to it (or maybe have @diksipav pick this up) ASAP.

--future-polymorphism-as-discriminated-unions

yeah, that was my first thought, but then I thought maybe in the next major we could allow you to still opt-into the old behavior, but now I'm thinking there is no advantage to keeping the old behavior around in the next major.

Yeah I don't see a reason to keep the old behavior long term.

scotttrinh · 2024-09-27T19:59:25Z

packages/generate/src/syntax/typesystem.ts

+/**
+ * Use declaration merging to set the {@link TypesystemOptions} via this
+ */
+// eslint-disable-next-line @typescript-eslint/no-empty-object-type
+export interface SetTypesystemOptions {}
+
+export type TypesystemOptions = {
+  future: {
+    /**
+     * Opt-in to the new discriminated union functionality for polymorphism.
+     */
+    polymorphismAsDiscriminatedUnions: SetTypesystemOptions extends {
+      future: { polymorphismAsDiscriminatedUnions: true };
+    }
+      ? true
+      : false;
+    /**
+     * Opt-in to strict __type__.name string literal unions.
+     */
+    strictTypeNames: SetTypesystemOptions extends {
+      future: { strictTypeNames: true };
+    }
+      ? true
+      : false;
+  };
+};
+


Do we still need this or can we use import future from "./future"; and get the type from the import?

I think it depends on what the DX goal is.

If the goal is to just use the cli flag and expect that future.ts is statically overridden, then no we don't need TypesystemOptions or the declaration merging on SetTypesystemOptions.

If the goal is to allow this to be set dynamically. Then yes we still need it.
In that case, devs would set the futures to true.

future.polymorphismAsDiscriminatedUnions = true; future.strictTypeNames = true;

And then denote to TS that they intend to do that with the declaration merge.

I don't think it makes sense for this to be dynamic in any way at all. You're either opting into this future behavior and will refactor all of your code to match it, or you're not. I also care about making sure the runtime behavior and types are aligned, so allowing (or really I guess encouraging) both seems dangerous.

Then yeah we don't need any of this. Just base it all of the futures object like you suggested

scotttrinh · 2024-09-27T20:00:08Z

packages/generate/src/syntax/typesystem.ts

+                    computeObjectShape<
+                      Pointer["target"]["__pointers__"] & Pointer["properties"],
+                      Element,
+                      TypesystemOptions["future"]["strictTypeNames"] extends true


So like,

Suggested change

TypesystemOptions["future"]["strictTypeNames"] extends true

(typeof future["strictTypeNames"]) extends true

etc

CarsonF · 2024-09-28T14:53:30Z

packages/generate/src/cli.ts

+      case "--future":
+        options.future = true;


I don't think this is sufficient. We need to be able to specify which futures you are opting into. I could be ok with future A and have updated my codebase to account for it. But then future B is added and I would be automatically opted into it as well which is a breaking change I wasn't ready for.

Yeah, @diksipav and I talked a bit about this yesterday and here are my thoughts:

From the perspective of a general feature of the generator package, this future behavior is similar to TypeScript's own strict behavior, which if you opt into directly is breaking between versions. It stands for some set of more granular configuration values, so I think this is useful in general.

From the perspective of this future behavior, I expect most (all?) folks to want to opt into both, but also would be fine exposing granular flags for each here, too. I don't want to add tests for each permutation since we plan on making this the only behavior in a future breaking change, but that does mean there might be bugs or poorly understood side-effects of having one or the other enabled, but not both.

I don't think we need to block on this, though, and I'm fine with trying to land this as-is now. I'll leave it up to @diksipav to decide if she wants to address this in this PR specifically, or as a follow-up PR.

hey lets merge this (once it's ready) and I'll create right away a follow up PR with individual future flags

@scotttrinh I'll merge this and start working on the PR with individual flags, if u think we should "swap" tests will do it in the next PR

jaclarke and others added 6 commits August 30, 2024 08:15

Implement better __type__.name type inference + infer result of pol…

82e1113

…ymorphic queries as discriminated union

Fix setToTsType from rebase

0ffc099

Replace {} with unknown

8b5a337

Fix turbo test not using $EDGEDB_SERVER_BIN

67837ff

Fix test assertions for newer test schema

4e2be6a

Restore some recent changes on shape types & stripping never (not pol…

31ec38b

…y unions)

CarsonF requested review from scotttrinh and jaclarke August 31, 2024 21:19

Strip properties with never values on polymorphic unions

7131057

CarsonF force-pushed the polymorphic-improvements branch from 3734b62 to 5b5d19c Compare September 3, 2024 13:04

CarsonF commented Sep 3, 2024

View reviewed changes

packages/generate/src/edgeql-js/generateObjectTypes.ts Outdated Show resolved Hide resolved

This comment was marked as resolved.

Sign in to view

CarsonF force-pushed the polymorphic-improvements branch from 5b5d19c to db0cbce Compare September 3, 2024 22:07

This comment was marked as resolved.

Sign in to view

Require opt-in to prevent it being a breaking change

b4dfe85

CarsonF force-pushed the polymorphic-improvements branch from db0cbce to 319a2cf Compare September 3, 2024 23:34

Fix ancestor types missing their concrete descendant type names

1ace1e0

CarsonF force-pushed the polymorphic-improvements branch from 319a2cf to 1ace1e0 Compare September 4, 2024 02:14

Hold typename keyed by symbol, so __typename__ isn't autocompleted.…

476d8e2

… `__type__.name` is the proper path.

CarsonF force-pushed the polymorphic-improvements branch 2 times, most recently from 92f8e59 to 50c8560 Compare September 4, 2024 03:53

Opt-in to strict strings for __type__.name

3be27a5

CarsonF force-pushed the polymorphic-improvements branch from 50c8560 to 3be27a5 Compare September 4, 2024 04:00

CarsonF commented Sep 4, 2024

View reviewed changes

CarsonF force-pushed the polymorphic-improvements branch from ddd9c6f to 530e5c6 Compare September 4, 2024 21:31

Avoid duplicating __typename in generated EdgeQL

3c1439c

CarsonF force-pushed the polymorphic-improvements branch from 530e5c6 to b9eff07 Compare September 4, 2024 22:26

CarsonF commented Sep 4, 2024

View reviewed changes

integration-tests/lts/select.test.ts Show resolved Hide resolved

Add failing test for discriminated unions from polymorphic queries vi…

814962a

…a type intersection expr

CarsonF force-pushed the polymorphic-improvements branch from b9eff07 to 814962a Compare September 4, 2024 22:36

CarsonF commented Sep 5, 2024

View reviewed changes

CarsonF mentioned this pull request Sep 5, 2024

[EdgeDB] Polymorphic queries & Engagement Repo SeedCompany/cord-api-v3#3284

Merged

scotttrinh approved these changes Sep 19, 2024

View reviewed changes

scotttrinh reviewed Sep 19, 2024

View reviewed changes

diksipav added 4 commits September 26, 2024 09:08

Merge branch 'master' into polymorphic-improvements

2073061

Add --future cli flug

582242c

Update typesystem to properly show __type__ name

c000eba

Fix formatting

a2f2c07

diksipav force-pushed the polymorphic-improvements branch from 1805d33 to a2f2c07 Compare September 27, 2024 17:51

scotttrinh reviewed Sep 27, 2024

View reviewed changes

CarsonF commented Sep 28, 2024

View reviewed changes

diksipav added 2 commits September 30, 2024 17:13

Use future object for future behaviour

b91241c

Add tests for the current, default behaviour

30e55ae

diksipav merged commit bcf4a8d into edgedb:master Sep 30, 2024
10 checks passed

CarsonF deleted the polymorphic-improvements branch September 30, 2024 16:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Polymorphic queries -> discriminated union types #1090

Polymorphic queries -> discriminated union types #1090

CarsonF commented Aug 31, 2024

CarsonF commented Sep 1, 2024

This comment was marked as resolved.

This comment was marked as resolved.

CarsonF Sep 4, 2024

CarsonF Sep 19, 2024

scotttrinh Sep 19, 2024

CarsonF Sep 5, 2024

scotttrinh Sep 19, 2024

scotttrinh left a comment

scotttrinh Sep 19, 2024

scotttrinh commented Sep 19, 2024

scotttrinh Sep 19, 2024

CarsonF Sep 19, 2024

scotttrinh Sep 19, 2024

CarsonF Sep 19, 2024 •

edited

Loading

scotttrinh Sep 19, 2024

CarsonF Sep 19, 2024

scotttrinh Sep 19, 2024

CarsonF Sep 19, 2024

scotttrinh Sep 19, 2024

CarsonF Sep 19, 2024

scotttrinh Sep 27, 2024

CarsonF Sep 27, 2024

scotttrinh Sep 28, 2024

CarsonF Sep 28, 2024

scotttrinh Sep 27, 2024

CarsonF Sep 28, 2024

scotttrinh Sep 28, 2024

diksipav Sep 30, 2024

diksipav Sep 30, 2024

	TypesystemOptions["future"]["strictTypeNames"] extends true
	(typeof future["strictTypeNames"]) extends true

Polymorphic queries -> discriminated union types #1090

Polymorphic queries -> discriminated union types #1090

Conversation

CarsonF commented Aug 31, 2024

CarsonF commented Sep 1, 2024

This comment was marked as resolved.

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scotttrinh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scotttrinh commented Sep 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CarsonF Sep 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CarsonF Sep 19, 2024 •

edited

Loading