prismicio / prismic-types

Type definitions for Prismic content, models, and APIs
https://prismic.io/docs/technologies/javascript
Apache License 2.0
11 stars 4 forks source link

Why "complex" Union type for content relationship field while it could simply be a Maybe (Value | null)? #50

Closed maapteh closed 1 year ago

maapteh commented 2 years ago

When im using the auto generated types i noticed for example:

const relatedFaq: EmptyLinkField<"Document"> | FilledContentRelationshipField<"faq", string, never>

where the emptyLinkField is not much like:

{ link_type: 'Document' }

my question is why you don't just pass null in that case? It makes it also way easier to consume down the line with if else. An empty document is not adding any value

github-actions[bot] commented 2 years ago

This issue has been labeled as a bug since it was created using the 🚨 Bug Report Template.

Hi there, thank you so much for the report!

Following our Maintenance Process, we will review your bug report and get back to you next Wednesday. To ensure a smooth review of your issue and avoid unnecessary delays, please make sure your issue includes the following:

If you have identified the cause of the bug described in your report and know how to fix it, you're more than welcome to open a pull request addressing it. Check out our quick start guide for a simple contribution process.

If you think your issue is a question (not a bug) and would like quicker support, please close this issue and forward it to an appropriate section on our community forum: https://community.prismic.io

- The Prismic Open-Source Team

lihbr commented 2 years ago

Hey @maapteh, thanks for your question!

Types from @prismicio/types describe data yielded by Prismic API v2. Within this API, a content relationship field can be one of two states: empty or filled.

In an ideal world, it could make sense for an empty field to be just null. The reason it's kept as an object is for various legacy and compatibility reasons, mainly that having an object was more permissive and prevented errors such as Cannot read properties of null (reading 'url').

Checking for a field to be empty or filled is a common pattern though, to achieve that, the best and battle-tested method to check for a field's state is by using @prismicio/helpers isFilled methods. These methods even come with great type narrowing.

import { isFilled } from "@prismicio/helpers";

/* ... */

if (isFilled.contentRelationship(doc.data.relatedFaq)) {
  /* ... */
}

Let us know if that answers your question :)

maapteh commented 2 years ago

Good evening Lucie, Well funny enough your helper is even casting it wrong, there it suddenly results not in a Maybe, but always a response

Screenshot 2022-10-05 at 22 40 07

So when not adding any content for this block, TS will trip and block my whole page at ln7

    const faqContent = prismicH.isFilled.contentRelationship(relatedFaq)
      ? relatedFaq
      : null;

    const [relatedFaqDocument] = await Promise.all([
      // THIS IS WRONG, TS SHOULD HELP ME WITH SAYING faqContent is nullable and faqContent?.id should be used
      ...(faqContent.id
        ? [client.getByID<FaqDocument>((relatedFaq as any).id)]
        : [null]),
    ]);

To be honest instead of empty making it just a dumb object is not adding any value, nullable objects are just to prevent stuff and tell you explicitly when to use foo?.. Now you add complexity to the consumer side.

I hope in a V2 or V3 you will not think too much about the past, thats why software like atlassian is so shitty to build your api's upon. Nullable when not found is perfect, because { link_type: 'Document' } is NOT really helpful. My typings see only that field now as the field which is there so i cant even do a check but have to write a special guard for it!! The helper is not a correct guard it suddenly allows also undefined and null as types to have it valued with the related content type. So no unfortunately this doesn't really help, makes it even worse (trusting the types and get runtime error when not filled in) :(

the error in the helper seems to be that suddenly it allows also | null | undefined as part of the return

maapteh commented 2 years ago

btw the helpers are also not tree shakable, whole objects ... :)

lihbr commented 2 years ago

Good evening Lucie, Well funny enough your helper is even casting it wrong, there it suddenly results not in a Maybe, but always a response

Unfortunately, I'm not able to reproduce it, see my setup here, type narrows as expected.

image

Happy to troubleshoot that further, if you'd like to too, I'll need more information about your setup and a minimal reproduction.

To be honest instead of empty making it just a dumb object is not adding any value, nullable objects are just to prevent stuff and tell you explicitly when to use foo?.. Now you add complexity to the consumer side.

I hope in a V2 or V3 you will not think too much about the past, thats why software like atlassian is so shitty to build your api's upon. Nullable when not found is perfect, because { link_type: 'Document' } is NOT really helpful. [...] (trusting the types and get runtime error when not filled in) :(

We agree with you, I'll forward that feedback to our product team.

btw the helpers are also not tree shakable, whole objects ... :)

Indeed, we've been working on improving our tree shakability recently as we have greater issues with it. For now I'm afraid that'll be an extra 350 bytes in the bundle.

maapteh commented 2 years ago

We take all relations from data. When you do so, your helper is lost :) So it only works in your example (where still it doesn't solve the typed issue when there is a document!!), but when for example destructing it will result in error

const { relation, ...here we take many more since we will all put them in next chain } = sink.data

const relBug = prismicH.isFilled.contentRelationship(relation)

now relBug is suddenly not optional :)

AND it leaves the relation itself UNTYPED :(

Property 'id' does not exist on type 'EmptyLinkField<"Document">'.

so not solving the main issue, having to write custom guards which is plain overhead....

lihbr commented 2 years ago

I'm still not able to reproduce, see my updated setup: types-50-2.zip

const { relation, ...here we take many more since we will all put them in next chain } = sink.data

const relBug = prismicH.isFilled.contentRelationship(relation)

now relBug is suddenly not optional :)

Indeed, isFilled* methods return a boolean representing the field's state, not the field itself.


Unless you provide a comprehensive reproduction I'll close this issue.

lihbr commented 1 year ago

Closing as the thread has become stale, happy to reopen it if you'd like to continue troubleshooting the above together~

esbeto commented 1 year ago

Hi, I arrived here by searching for the function I was using isFilled.contentRelationship, and typescript doesn't seem to understand that the value is filled.

I'm using:

   "@prismicio/types": "^0.2.4",
    "prismic-ts-codegen": "^0.1.5",

Here's my code:

import { isFilled } from "@prismicio/helpers";
import { BlogArticleDocument, BlogCategoryDocument } from "~~/types.generated";

function getCategoryLabel(article: BlogArticleDocument) {
  const category = article.data.blog_category;
  const blogCategory = isFilled.contentRelationship(category) ? category : null;

  if (blogCategory) {
    return blogCategory.data.label;
  }

  return "";
}

But I'm getting the following type error:

'blogCategory.data' is of type 'unknown'.ts(18046)
Screenshot 2022-12-17 at 01 15 12

I'm not sure how to get the category label field. Can you help me?

esbeto commented 1 year ago

OK, I just fixed it, I looked at the documentation, specifically the last example here: https://prismic.io/docs/technical-reference/prismicio-client#typescript

Just for reference, this was the code that pleased Typescript:

import { isFilled } from "@prismicio/helpers";
import { BlogArticleDocument, BlogCategoryDocument } from "~~/types.generated";

function getCategoryLabel(article: BlogArticleDocument) {
  const category = article.data.blog_category as typeof article.data.blog_category & {
    data: Pick<BlogCategoryDocument['data'], "label">
  }

  const blogCategory = isFilled.contentRelationship(category) ? category : null;

  if (blogCategory) {
    return blogCategory.data.label;
  }

  return "";
}

Simplified:

function getCategoryLabel(article: BlogArticleDocument) {
  const category = article.data.blog_category as typeof article.data.blog_category & {
    data: Pick<BlogCategoryDocument['data'], "label">
  }

  return isFilled.contentRelationship(category) ? category.data.label || "" : "";
}
lihbr commented 1 year ago

Hey @esbeto, thanks for sharing your findings!

Indeed, the API is not really convenient to use when it comes to content relationship fetchLinks/graphQuery usage. The above you shared is the current recommended way of dealing with it.

We'll try to explore if a better API can be made available :)