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

fix(gatsby-image): Fix typings for fixed and fluid props #24767

Merged
merged 6 commits into from
Aug 12, 2020

Conversation

schwartzmj
Copy link
Contributor

Description

GatsbyImage should require one of "fluid" or "fixed". Currently they are both optional.

Documentation

Self documenting types. Not changing file exports.

Related Issues

GatsbyImage should require one of "fluid" or "fixed". Currently they are both optional.
@schwartzmj schwartzmj requested a review from a team as a code owner June 4, 2020 05:11
@gatsbot gatsbot bot added the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Jun 4, 2020
fix typo extends GatsbyImage -> GatsbyImageOptionalProps
@schwartzmj
Copy link
Contributor Author

I believe this is important because if an <Img /> component receives an undefined or null fixed or fluid prop, Typescript won't yell at you.

Once the site is live, the page crashes to a white screen with error Cannot read property '0' of undefined.

This isn't something that can always be caught in development because a template may pull in images and generate pages dynamically.

@ascorbic ascorbic self-assigned this Jun 4, 2020
@ascorbic ascorbic added topic: TypeScript migration and removed status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer labels Jun 4, 2020
@ascorbic
Copy link
Contributor

ascorbic commented Jun 4, 2020

This is great. It is technically a breaking change though, because people could be using the old deprecated props resolutions and sizes. Could you mitigate that by adding @deprecated jsdoc comments to those? e.g.

  /**
   * @deprecated Use `fixed`
   */
  resolutions?: FixedObject

packages/gatsby-image/index.d.ts Outdated Show resolved Hide resolved
schwartzmj and others added 2 commits June 4, 2020 09:34
@schwartzmj
Copy link
Contributor Author

Sounds good, I've made those adjustments. If you have any other suggestions let me know!

@ascorbic
Copy link
Contributor

@schwartzmj Could you check the types in the IE polyfill. It's currently failing typechecks

@schwartzmj
Copy link
Contributor Author

schwartzmj commented Jun 18, 2020

Could you check the types in the IE polyfill. It's currently failing typechecks

Good catch. I've made some changes to account for that. It looks like the "Gatsby Build Service" is failing. Anything on my end I need to fix?

Copy link
Contributor

@sidharthachatterjee sidharthachatterjee left a comment

Choose a reason for hiding this comment

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

This is good to go. Thanks @schwartzmj!

@sidharthachatterjee sidharthachatterjee changed the title fixed and fluid props aren't actually optional. one is required. fix(gatsby-image): Fix typings for fixed and fluid props Aug 12, 2020
@sidharthachatterjee sidharthachatterjee merged commit 08e0aa1 into gatsbyjs:master Aug 12, 2020
@henrikra
Copy link

This change made TypeScript compiler fail with styled-components. With gatsby-image up to 2.4.15 it compiles fine but soon as I update to 2.4.16 or newer compilation fails. Can you guys fix it?

Here is the code
image

@antoinerousseau
Copy link
Contributor

antoinerousseau commented Sep 14, 2020

As @henrikra mentioned:

Until gatsby-image 2.4.15, I was doing this, using styled-components:

const Photo = styled(Image)`
  /* some css */
`

And then

<Photo fluid={photo.fluid} />

But since gatsby-image 2.4.16 (this PR), I'm getting this TypeScript error:

 No overload matches this call.
  Overload 1 of 2, '(props: Pick<Pick<(GatsbyImageFluidProps & { objectFit?: "fill" | "none" | "contain" | "cover" | "scale-down"; objectPosition?: string; } & RefAttributes<GatsbyImageWithIEPolyfill>) | (GatsbyImageFixedProps & ... 1 more ... & RefAttributes<...>), "ref" | ... 23 more ... | "objectPosition"> & Partial<...>, "ref" | ... 23 more ... | "objectPosition"> & { ...; } & { ...; } & { ...; }): ReactElement<...>', gave the following error.
    Type '{ className: string; fluid: FluidObject; alt: string; }' is not assignable to type 'IntrinsicAttributes & Pick<Pick<(GatsbyImageFluidProps & { objectFit?: "fill" | "none" | "contain" | "cover" | "scale-down"; objectPosition?: string; } & RefAttributes<...>) | (GatsbyImageFixedProps & ... 1 more ... & RefAttributes<...>), "ref" | ... 23 more ... | "objectPosition"> & Partial<...>, "ref" | ... 23 mor...'.
      Property 'fluid' does not exist on type 'IntrinsicAttributes & Pick<Pick<(GatsbyImageFluidProps & { objectFit?: "fill" | "none" | "contain" | "cover" | "scale-down"; objectPosition?: string; } & RefAttributes<...>) | (GatsbyImageFixedProps & ... 1 more ... & RefAttributes<...>), "ref" | ... 23 more ... | "objectPosition"> & Partial<...>, "ref" | ... 23 mor...'.
  Overload 2 of 2, '(props: StyledComponentPropsWithAs<typeof GatsbyImageWithIEPolyfill, any, {}, never>): ReactElement<StyledComponentPropsWithAs<typeof GatsbyImageWithIEPolyfill, any, {}, never>, string | ... 1 more ... | (new (props: any) => Component<...>)>', gave the following error.
    Type '{ className: string; fluid: FluidObject; alt: string; }' is not assignable to type 'IntrinsicAttributes & Pick<Pick<(GatsbyImageFluidProps & { objectFit?: "fill" | "none" | "contain" | "cover" | "scale-down"; objectPosition?: string; } & RefAttributes<...>) | (GatsbyImageFixedProps & ... 1 more ... & RefAttributes<...>), "ref" | ... 23 more ... | "objectPosition"> & Partial<...>, "ref" | ... 23 mor...'.
      Property 'fluid' does not exist on type 'IntrinsicAttributes & Pick<Pick<(GatsbyImageFluidProps & { objectFit?: "fill" | "none" | "contain" | "cover" | "scale-down"; objectPosition?: string; } & RefAttributes<...>) | (GatsbyImageFixedProps & ... 1 more ... & RefAttributes<...>), "ref" | ... 23 more ... | "objectPosition"> & Partial<...>, "ref" | ... 23 mor...'.

86             <Photo fluid={photo.fluid} />
                      ~~~~~

@moshie
Copy link
Contributor

moshie commented Dec 4, 2020

When using the GatsbyImageProps as a type on an image you run into:

Property 'fluid' does not exist on type 'GatsbyImageFixedProps'

https://www.typescriptlang.org/docs/handbook/unions-and-intersections.html#unions-with-common-fields

@aquelehugo
Copy link

I think the issue goes back to styled-components: styled-components/styled-components#3385

@henrikra
Copy link

Good finding @aquelehugo .

Please guys show your support at styled-components/styled-components#3385 so we get this annoying bug resolved!

@henrikra
Copy link

There is now PR which fixes this problem! DefinitelyTyped/DefinitelyTyped#51252

pragmaticpat pushed a commit to pragmaticpat/gatsby that referenced this pull request Apr 28, 2022
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.

None yet

7 participants