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.

Sign up for GitHub

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

Jump to bottom

Make brandWithType return the branded object #667

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Make brandWithType return the branded object #667

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
43db454
Make brandWithType return the branded object
XiNiHa Nov 13, 2022
dad1ebf
Add changeset
XiNiHa Nov 13, 2022
68c0858
fix
XiNiHa Nov 14, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/dull-buses-fix.md
View file
Open in desktop
@@ -0,0 +1,5 @@
---
'@pothos/core': minor
---

Make `brandWithType` return the branded object
17 changes: 15 additions & 2 deletions packages/core/src/utils/index.ts
View file
Open in desktop
Expand Up @@ -68,15 +68,28 @@ builder.objectType('MyObject', {
}
}

export function brandWithType<Types extends SchemaTypes>(val: unknown, type: OutputType<Types>) {
type BrandWithTypeResult<T, Types extends SchemaTypes> = T extends Record<string, unknown>
? {
[K in keyof T | typeof typeBrandKey]: K extends keyof T
? T[K]
: OutputType<Types>
}
: T;

export function brandWithType<T, Types extends SchemaTypes>(
val: T,
type: OutputType<Types>,
): BrandWithTypeResult<T, Types> {
Copy link
Owner

Choose a reason for hiding this comment

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

I'm not sure adding the brand field to the type here makes sense, it's intended as an invisible side effect, I would probably just return T directly

Copy link
Author

Choose a reason for hiding this comment

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

But in that case the type error still persists (since the field resolver expects the returned value to have the key attached)

Copy link
Owner

Choose a reason for hiding this comment

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

Do you have an example of how you are trying to use this?

Copy link
Author

Choose a reason for hiding this comment

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

Something like this:

author: t.field({
  type: UserOrTeam,
  resolve: async (root) => {
    if (root.authorType === 'USER')
      return brandWithType(
        await prisma.user.findUnique({ where: { id: root.authorId } }),
        User
      )
    else
      return brandWithType(
        await prisma.team.findUnique({ where: { id: root.authorId } }),
        Team
      )
  }
})

Copy link
Owner

Choose a reason for hiding this comment

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

The pothos type system doesn't have any concept of typeBrandKey. Adding it to the return type here, should not be affecting how this field is type-checked. I am fairly sure that if you update brandWithType to just return T (which allows you to get rid of the Types parameter as well) should still allow this to work correctly without type errors.

Copy link
Owner

Choose a reason for hiding this comment

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

there is an existing WithBrand type in the prisma plugin you could copy or move instead. I think the type used above is still a bit over-complicated

Copy link
Author

Choose a reason for hiding this comment

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

Ooooh actually .addBrand was the one I was looking for. Maybe I should close the PR then 🤔

Copy link
Sponsor Contributor

Choose a reason for hiding this comment

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

Hello. After updating Pothos I started running into this error, and I found the solution here. Previously, I had no errors. This is happening when dealing with a union type.

I was under the impression that this sort of thing was handled by isTypeOf and friends, and obviously until this update, things worked. What is the reason that this is needed all of a sudden, and why is there not a single mention of this in the documentation here: https://pothos-graphql.dev/docs/plugins/prisma ? The word brand doesn't occur once.

Copy link
Owner

Choose a reason for hiding this comment

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

yes, this definitely needs some docs. Sorry about that.

So with isTypeOf checks, this technically isn't needed. The issue is that writing good isTypeOf checks for Prisma objects is almost impossible without adding a column to your DB with the type name.

The way the brands work currently is roughly like this:

When you create a union, if the union doesn't have a resolveType resolver, it will check to see if any of the types declared a custom "abstract shape", which is basically a different type to use when the type is returned as part of a union. Pothos default resolveType checks for a brands on objects that are part of a union as a way to identify them automatically.

This was always how the node/nodes interfaces worked for prisma objects, but the more recent change was intended to make it easier to build your own interfaces/unions that used prisma objects. Unfortunately I forgot to document them.

If you have working isTypeOf checks, I can see why this change is pretty inconvenient. The issue is that pothos can optimize selections to only load the fields that were queried, and consistently identifying objects in an isTypeOf check can be tricky because sometimes the only thing selected is the id.

If you have a resolveType check the expected type should not require branding, but for everything else, it should be easy to add, you basically just need to wrap your prisma calls like: YourType.addBrand(prisma.yourTyoe.findMany(...)). Which should also allow you to remove the isTypeOf checks. If you have a good way of writing isTypeOf checks let me know, this is something I have never found a good way to do

Copy link
Sponsor Contributor

Choose a reason for hiding this comment

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

Thanks a lot for your detailed reply!

Yes, that's actually a brilliant idea. I've run into the exact same problems you describe with isTypeOf checks, and in a couple of places I'm doing some funky stuff to get it working.

Thanks for all your hard work as well -- pothos is absolutely great, and I really appreciate the thought and engineering that goes into it.

if (typeof val !== 'object' || val === null) {
return;
return val as BrandWithTypeResult<T, Types>;
}

Object.defineProperty(val, typeBrandKey, {
enumerable: false,
value: type,
});

return val as BrandWithTypeResult<T, Types>;
}

export function getTypeBrand(val: unknown) {
Expand Down