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

Feature request - Missing @types/react-imgix #356

Open
dustinbrink opened this issue May 7, 2019 · 15 comments
Open

Feature request - Missing @types/react-imgix #356

dustinbrink opened this issue May 7, 2019 · 15 comments

Comments

@dustinbrink
Copy link

Is your feature request related to a problem? Please describe.
Would like a published @types/react-imgix for us typescript developers.

@sherwinski
Copy link
Contributor

Hi @dustinbrink, thanks for the request. I think this is a good idea, although I am not super familiar with the process of adding typescript support for a project. I did find some guidelines after searching around that seem related. Since you're likely more familiar with the process, would you mind pointing me to some resources you know of that could help us get started on this?
Thanks in advance!

@hagmic
Copy link

hagmic commented Nov 11, 2019

I've done some initial work to start the conversation on this issue. The props in this README didn't match up exactly with the actual component implementation, so I'd love some feedback on how the props should be structured/exported for these types. For now, all imgixParams allow string | number | boolean. I'm not an expert in all of the params, but they could be further refined to match the actual API.

DefinitelyTyped/DefinitelyTyped#40289

@frederickfogerty
Copy link
Contributor

Thanks @hagmic! This is a big step forward. I have a request for the DefinitelyTyped PR - do you think that myself, @sherwinski, and @jayeb could also somehow get something like "maintenance access" to @types/react-imgix. I'm not quite sure how DefinitelyTyped works, but it will be important for us to keep these types somewhat up to date as we update this library.

The props in this README didn't match up exactly with the actual component implementation, so I'd love some feedback on how the props should be structured/exported for these types.

I think we should go with the code as the source-of-truth unless the code is obviously wrong, and update the README to match the code. This prevents us from having to release a breaking change for just changing the types of the props 👍.

For now, all imgixParams allow string | number | boolean.

For some help here, there's some prior work here (not for this library though). This repo is our source-of-truth for imgix params, and includes types too. https://github.com/imgix/imgix-url-params/blob/master/dist/parameters.json

Unsplash also did some typescript work for their own imgix library, maybe some of their types will be useful: https://github.com/unsplash/ts-imgix/blob/master/src/index.ts

@esetnik
Copy link

esetnik commented Dec 10, 2019

@frederickfogerty if you want to maintain control of the types it would be better to accept a PR directly to this repo for the types to be included directly in the project. See https://www.typescriptlang.org/docs/handbook/declaration-files/publishing.html Then you can require types to be updated whenever changes are made and typescript users of your package won't need to install any @types dependencies.

@esetnik
Copy link

esetnik commented Dec 10, 2019

I've done some initial work to start the conversation on this issue. The props in this README didn't match up exactly with the actual component implementation, so I'd love some feedback on how the props should be structured/exported for these types. For now, all imgixParams allow string | number | boolean. I'm not an expert in all of the params, but they could be further refined to match the actual API.

DefinitelyTyped/DefinitelyTyped#40289

@hagmic I tested this out and it looks good. The only issue I had was:

{
	"owner": "typescript",
	"code": "2322",
	"severity": 8,
	"message": "Type '{ sizes: string; imgixParams: { ar: string; }; alt: string; src: string; }' is not assignable to type 'IntrinsicAttributes & Pick<Pick<SharedImigixAndSourceProps & RefAttributes<Imgix>, \"ref\" | \"key\" | \"className\" | \"height\" | \"sizes\" | \"src\" | \"width\" | \"disableQualityByDPR\" | ... 5 more ... | \"htmlAttributes\"> & Partial<...>, \"ref\" | ... 12 more ... | \"htmlAttributes\"> & { ...; } & { ...; } & { ...; }'.\n  Property 'alt' does not exist on type 'IntrinsicAttributes & Pick<Pick<SharedImigixAndSourceProps & RefAttributes<Imgix>, \"ref\" | \"key\" | \"className\" | \"height\" | \"sizes\" | \"src\" | \"width\" | \"disableQualityByDPR\" | ... 5 more ... | \"htmlAttributes\"> & Partial<...>, \"ref\" | ... 12 more ... | \"htmlAttributes\"> & { ...; } & { ...; } & { ...; }'.",
	"source": "ts",
	"startLineNumber": 46,
	"startColumn": 10,
	"endLineNumber": 46,
	"endColumn": 20
}

In the following:

        <Imgix
          sizes="100vw"
          imgixParams={{ ar: '16:9' }}
          alt={title}
          src={getClassImageSrc(photoId)}
        />

@ryami333
Copy link

In my experience, the "simplest" way to do this is to actually migrate your own project to Typescript, and then publish a "declaration file" as part of your build-step. That way, types don't require any extra consideration with each update/publish, they're just a build-artifact.

@esetnik
Copy link

esetnik commented Dec 10, 2019

In my experience, the "simplest" way to do this is to actually migrate your own project to Typescript, and then publish a "declaration file" as part of your build-step. That way, types don't require any extra consideration with each update/publish, they're just a build-artifact.

Yes, but then the maintainers of this repository need to be familiar with Typescript implementation and do a lot of work to migrate the project. The advantage of separate typings is that the community of users who cares about types can maintain them, and the original authors don't need to do anything.

@ryami333
Copy link

ryami333 commented Dec 10, 2019

The advantage of separate typings is that the community of users who cares about types can maintain them, and the original authors don't need to do anything.

Yes, but the advantage of migrating to typescript is that (once the migration is performed) neither party needs to "maintain" them.

Yes, but then the maintainers of this repository need to be familiar with Typescript implementation and do a lot of work to migrate the project

Depends on your definition of "a lot of work". I'd argue that this would be a pretty trivial migration. In fact I'd perhaps even volunteer to do it, if the maintainers indicated an appetite to consider that PR.

@sherwinski
Copy link
Contributor

I'm glad to see a lot of activity and interest in this, and as project maintainers we definitely want to make this happen. I would like to weigh in on a few things though:

@frederickfogerty if you want to maintain control of the types it would be better to accept a PR directly to this repo for the types to be included directly in the project. See typescriptlang.org/docs/handbook/declaration-files/publishing.html Then you can require types to be updated whenever changes are made and typescript users of your package won't need to install any @types dependencies.

With my understanding of the different options here, I'm most in favor of taking this approach. This is what we did in imgix-core-js and it seems to have worked fairly well. On top of that, @esetnik's point about the maintainers needing to be familiar with Typescript is an important one that I think we need to consider. I think following this approach would also help us get something out much more quickly for this project.

Regarding the work @hagmic did on the DefinitelyTyped PR: while having a list of imgix parameters/types is what we're striving for here, it actually results in a lot more overhead for us as maintainers. Maintaining a list of parameters is a pattern that we try to avoid throughout our libraries, as we have tried and been burned by it in the past. You can imagine how quickly things can get out of hand across multiple projects if part of our deployment process requires keeping each of these lists in sync. That's why, at least initially, I think we should start with something more simple and iterate from there.

I would love it if eventually part of our build process included syncing the declaration file with the current state of imgix-url-params. But that's a separate undertaking altogether.

@esetnik
Copy link

esetnik commented Dec 11, 2019

I'm glad to see a lot of activity and interest in this, and as project maintainers we definitely want to make this happen. I would like to weigh in on a few things though:

@frederickfogerty if you want to maintain control of the types it would be better to accept a PR directly to this repo for the types to be included directly in the project. See typescriptlang.org/docs/handbook/declaration-files/publishing.html Then you can require types to be updated whenever changes are made and typescript users of your package won't need to install any @types dependencies.

With my understanding of the different options here, I'm most in favor of taking this approach. This is what we did in imgix-core-js and it seems to have worked fairly well. On top of that, @esetnik's point about the maintainers needing to be familiar with Typescript is an important one that I think we need to consider. I think following this approach would also help us get something out much more quickly for this project.

Regarding the work @hagmic did on the DefinitelyTyped PR: while having a list of imgix parameters/types is what we're striving for here, it actually results in a lot more overhead for us as maintainers. Maintaining a list of parameters is a pattern that we try to avoid throughout our libraries, as we have tried and been burned by it in the past. You can imagine how quickly things can get out of hand across multiple projects if part of our deployment process requires keeping each of these lists in sync. That's why, at least initially, I think we should start with something more simple and iterate from there.

I would love it if eventually part of our build process included syncing the declaration file with the current state of imgix-url-params. But that's a separate undertaking altogether.

Actually the work @hagmic did is nearly perfect as far as I’m concerned. There’s a large value in having all the available parameters enumerated as they have done because it makes the entire project type-safe with regard to common developer errors like typos in parameter names or passing a value that was subsequently removed from the project. The best part about keeping the types in the project itself instead of DefinitelyTyped is that types are versioned with the project so new releases of the project automatically match new types and the user doesn’t need to keep them in sync. A large number of issues are raised in many projects because the types in DefinitelyTyped no longer match the underlying project or people install the wrong versions. The best part is that the work @hagmic did on DefinitelyTyped could be PRed into this project in a matter of 5 minutes and the problem would be solved for everybody. So that’s my suggestion.

@esetnik
Copy link

esetnik commented Dec 11, 2019

With regard to the maintenance of parameters, I wouldn’t worry about it too much. In the worst case scenario you release an update with a missing or incorrect type definition for a new parameter and the community of users will quickly submit a PR with the fix. I don’t think this is something the maintainers of this project need to be overly concerned with. The types don’t have to be 100% perfect all the time to still provide a huge value as long as the common scenarios are covered.

@sherwinski
Copy link
Contributor

Hey everyone,
So we discussed this matter internally and have a plan for how we want to tackle this going forward. The key points that I want to highlight are that we want to 1) keep the types internal to this project and 2) be able to build the enumeration of parameters off a single, accurate artifact at the time of each release. To get to this point eventually, here is our proposed plan:

Step 1: Build out types for all the react-related components/properties in this project, excluding parameters for imgixParams for the time being. That basically includes everything from L201 and on from @hagmic’s declaration file. This should give us the quickest path to getting minimal working types to users while we work on Step 2.

Step 2: In the meantime, @jayeb and I will work on a way to generate an enumeration of imgix parameter types from imgix-url-params. This will produce an distributable artifact that will be updated with each new imgix release. We plan to put a great amount of detail and care into these type definitions so that they will be specific and act as a source of truth on what parameters map to what values.

Step 3: Once we reach this point, we can import the artifact from Step 2 as a types dependency for this project. We’ll need to do a little bit of work to hook things up and set up tests, but once completed I think we can trust that we are providing the best type definitions we can.

I'm happy to answer any questions in the meantime.

@bernardchu
Copy link

I realize I'm quite late to the party here, but I recently started using react-imgix with TypeScript and have found a couple small issues related to the TS definitions. I could open new issues if you'd prefer but I figured I'd start in this thread first.

  1. typos - ImigixParams and SharedImigixAndSourceProps spell Imgix incorrectly
  2. params are almost all enumerated but txt64 (and, I'm assuming, all other foo64 params) are missing. I see that there's been some discussion regarding the pain of maintaining an exhaustive list of params in the TS definitions (and elsewhere presumably) so maybe this is an intentional omission at the moment. For now anyway there's always the @tsignore workaround so it's no biggie, as the functionality seems fine once the TS error is suppressed.

I'm using "@types/react-imgix": "9.3.1".

@frederickfogerty
Copy link
Contributor

Hey @bernardchu, thanks for commenting on this. We are actively working on updating the TS definitions so we will keep these comments in mind!

@wweaver
Copy link

wweaver commented Jul 1, 2022

I created a PR to hopefully fix the type issues in react 18
DefinitelyTyped/DefinitelyTyped#61028

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

No branches or pull requests

8 participants