Skip to content

Conversation

@atombrenner
Copy link
Contributor

This PR adds a typescript definition file the the package, see #27

It allows typescript users to directly use this package without installing another dependency from DefinitelyTyped.
It allows the maintainer to add/change/remove types and keep the type definitions in sync with the implementation.

In addition to the existing type annotation at DefinitelyTyped, most tags have now more specific types, e.g. ImageWidth is now of type number which is more precise than unknown.

The tag names were automatically generated from tag.js. During that processed I noticed, that tag.s uses the same tag name for different tags (e.g. the WhiteBalance tag). That makes typing hard, because at runtime it depends on the image which tag is returned. The last encountered one overwrites existing tags. I don't know if this is a bug or a feature, but in those cases we can only return a generic tag type, which is

type GenericTag = number | number[] | string | Buffer | null;

@lovell
Copy link
Collaborator

lovell commented May 18, 2023

Thank you very much for the PR Christian.

/cc @akwodkiewicz as they provided the current definitions via DefinitelyTyped/DefinitelyTyped#62942

@atombrenner
Copy link
Contributor Author

Hey @lovell, thanks for the tip to use tsd to test the index.d.ts. It found a bug and and also helped improving the declaration file, it feels like TDD for typings 😄

Handling the deprecation process seems pretty straight forward, so I can take care if @akwodkiewicz is not interested.

Copy link
Collaborator

@lovell lovell left a comment

Choose a reason for hiding this comment

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

Thanks for the tests, one question inline otherwise this looks great.

Copy link
Collaborator

@lovell lovell left a comment

Choose a reason for hiding this comment

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

LGTM, thank you. I'll leave this PR open for now to let @akwodkiewicz and @devongovett have a chance to comment if they wish.

@lovell
Copy link
Collaborator

lovell commented Jun 6, 2023

@atombrenner Should we close this PR and continue discussion using #30 ?

@atombrenner
Copy link
Contributor Author

@lovell I agree, as #30 is based on this PR and typing is nearer to standard tag names.

@lovell
Copy link
Collaborator

lovell commented Jul 11, 2023

Closing as superseded by #30

@lovell lovell closed this Jul 11, 2023
@akwodkiewicz
Copy link

Sorry for joining this late to the conversation. My contribution to DT was a "path of least resistance"-approach to typings. I wanted to have the package behave correctly in my side project, and I decided to share this so that nobody will be forced to recreate the very same types themselves.

I was counting on somebody to upgrade my work when they realized there's more to be done, though 😄 It's great you decided to include the types within the package.

Is there anything I should do regarding DT (e.g. delete types from there)?

@lovell
Copy link
Collaborator

lovell commented Jul 13, 2023

@akwodkiewicz Thanks for the background, feel free to review #30 if it might be of interest to you. Once that PR is merged and published then I'm sure any help to deprecate/remove the DT-based definitions would be most welcome.

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.

3 participants