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

loosen hotspot & crop types to match sanity typegen #54

Closed
wants to merge 1 commit into from

Conversation

RJWadley
Copy link

@RJWadley RJWadley commented Dec 5, 2024

Sanity typegen spits out types that look like this:

export type SanityImageCrop = {
  _type: "sanity.imageCrop"
  top?: number
  bottom?: number
  left?: number
  right?: number
}

export type SanityImageHotspot = {
  _type: "sanity.imageHotspot"
  x?: number
  y?: number
  height?: number
  width?: number
}

with the current type these can't be used directly - you need to do a little typecheck dance:

<SanityImage
  // check & provide fallbacks
  hotspot={
    src.hotspot
      ? { x: src.hotspot.x ?? 0, y: src.hotspot.y ?? 0 }
      : undefined
  }
  // or check each property individually
  crop={
    src.crop?.top && src.crop.left && src.crop.bottom && src.crop.right
      ? {
          top: src.crop.top,
          left: src.crop.left,
          bottom: src.crop.bottom,
          right: src.crop.right,
        }
      : undefined
  }
/>

this would loosen the type to allow usage like this:

<SanityImage
  hotspot={src.hotspot}
  crop={src.crop}
/>

I don't think sanity would ever spit out an image with a partially defined crop or hotspot value, but I've also added fallbacks where needed to prevent runtime errors if they did

@coreyward
Copy link
Owner

Hmm, is it not viable to update Sanity typegen to generate accurate types? A crop or hotspot with partial values is invalid and I'd much rather keep the types stricter to avoid an issue where someone accidentally misses a value and the fallback sets it to 0 silently and creates an unexpected result.

@RJWadley
Copy link
Author

RJWadley commented Dec 13, 2024

true, some sort of validation & error would be better than a silent fallback. sanity typegen itself is actually working as expected, rather sanity's schema extraction marks these properties as optional:

"x": {
  "type": "objectAttribute",
  "value": {
    "type": "number"
  },
  "optional": true
},

After a bit of investigation, I think this line in the schema extractor marks every attribute optional (passing --enforce-required-fields has no effect on hotspot and crop attributes). this could also very easily be wrong - I'm not familiar with sanity schema extraction at all

@coreyward
Copy link
Owner

Yeah, seems like the schema extraction logic there is incorrect. The Sanity studio never sets a hotspot or crop value with only partial properties, so even if it's a draft document, it's safe to rely on the full set of properties being available. Unfortunately I don't think it's viable to make this any better from within sanity-image—adapting the types to not complain just covers up the problem and potentially opens up runtime bugs.

@RJWadley
Copy link
Author

closing in favor of sanity-io/sanity#8060

@RJWadley RJWadley closed this Dec 13, 2024
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.

2 participants