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

feat(types): add types file #2398

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

feat(types): add types file #2398

wants to merge 8 commits into from

Conversation

rubiin
Copy link
Member

@rubiin rubiin commented May 9, 2024

This PR adds types file within the repo itself .
The types file is derived from https:/DefinitelyTyped/DefinitelyTyped/tree/master/types/validator
with some modifications

Checklist

  • PR contains only changes related; no stray files, etc.
  • README updated (where applicable)
  • Tests written (where applicable)
  • References provided in PR (where applicable)

@rubiin
Copy link
Member Author

rubiin commented May 9, 2024

Having types file here will make sure the types and codebase are in sync.

@profnandaa
Copy link
Member

LGTM. Could you please also add some guidelines on how to use, on the README? Perhaps adding a section for Types?

@rubiin
Copy link
Member Author

rubiin commented Jun 1, 2024

Sure thing. I will update the readme for this

@rubiin
Copy link
Member Author

rubiin commented Jun 1, 2024

Since the types get installed along with the library, there is no need to install @types/validator package anymore for typescript support. I have added a small section on readme for that but we should also point this on the realease notes @profnandaa

README.md Outdated Show resolved Hide resolved
@jorgevrgs
Copy link
Contributor

Having types file here will make sure the types and codebase are in sync.

+1 for this one, I've been checking the latest @types/validator version doesn't include the type for isAbaRouting. It seems this PR is outdated because this type is missing here too.

@rubiin
Copy link
Member Author

rubiin commented Jun 8, 2024

Yeah . Since we have types file here , we dont have to depend on the @types/ packages . They can be slow and outdated so self managing is a better option

Copy link
Contributor

@pano9000 pano9000 left a comment

Choose a reason for hiding this comment

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

not a TypeScript expert, so not sure, what is usually preferred: one huge file or several smaller ones - but just from a gut feeling, I think it might be easier to many smaller ones, instead of one huge one?
The DefinitelyTyped version was also going with many small files, from what I can see.
but feel free to diregard my comment, if in TS world things are handled differently

README.md Outdated Show resolved Hide resolved
rubiin and others added 2 commits August 25, 2024 07:18
@rubiin
Copy link
Member Author

rubiin commented Aug 25, 2024

For now we can ship this and on later date , break it down on smaller files.

README.md Show resolved Hide resolved
README.md Show resolved Hide resolved
Comment on lines +9 to +12
/**
* Check if the string is a valid [ISO 4217](https://en.wikipedia.org/wiki/ISO_4217) officially assigned currency code.
*/
export function isISO4217(str: string): boolean;
Copy link
Contributor

@ST-DDT ST-DDT Aug 26, 2024

Choose a reason for hiding this comment

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

Please run a formatter like prettier on this file, so it is formatted consistently and I don't have to comment on those parts.

Copy link
Contributor

Choose a reason for hiding this comment

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

If you resolve my comments without actually changing anything, please at least tell me why.

Copy link
Member

Choose a reason for hiding this comment

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

@rubiin can you take another look at the comments by @ST-DDT ? Also the resolved ones.
You can add index.d.ts to the list that is being linted by https:/validatorjs/validator.js/blob/master/package.json#L58-L59

Copy link
Member Author

Choose a reason for hiding this comment

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

sure thing will do sometime later

Choose a reason for hiding this comment

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

You need to provide a "types" field in the package.json for the types to actually work. Checkout the TypeScript Handbook regarding that topic.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch, will update it asap

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants