validatorjs / validator.js

String validation
MIT License
23.11k stars 2.31k forks source link

discussion: support for TypeScript #1271

Open profnandaa opened 4 years ago

profnandaa commented 4 years ago

This is a fresh take at #1261

Re-posting my initial thoughts:

So, a rewrite is out of question. Looks like viable proposals we have right now are:

1) Add types alongside the library (for easy maintenance), as opposed to doing it from DT 2) Find a way of keeping DT up-to-date (uphill task coz of lack of autonomy) 3) [my addition] Have a separate repo here in the validatorjs org specifically for types (which can be mirrored with DT)

Personally I'm leaning towards 3. I'm also trying to:

Let me know what you folks think.

/cc. @tux-tn @johannesschobel

profnandaa commented 4 years ago

Cross-ref #789 #247 #662 #1194

deadcoder0904 commented 4 years ago

Would love support for this :)

How do I use Tree-Shaking with TS if anyone's got ideas?

I have already installed @types/validator from DT & would love to incorporate tree-shaking as well with it.

It currently gives errors if I directly use it from es/* libraries as they don't have types associated with them

rcollette commented 4 years ago

@deadcoder0904 - I imported like

import isEmail from 'validator/lib/isEmail';

That's as small as you'll get for tree shaking with TS alone, otherwise you need Webpack or something similar to get tree shaking.

rcollette commented 4 years ago

@profnandaa - I'm curious why the comment about not wanting to rewrite. JS is TS. There shouldn't be a need to rewrite anything.

You could just rename the files to TS and apply type information. You have the option to compile to multiple targets if you want ES5, etc.

If you don't want to write TS at all, you could use JSDoc, perform type checking with the compiler to ensure usage is consistent, and generate type declaration files from the .js

https://www.typescriptlang.org/docs/handbook/type-checking-javascript-files.html https://voxpelli.com/2019/10/use-type-script-3-7-to-generate/

AubreyHewes commented 4 years ago

@profnandaa You should not mirror own types to TD.. TD started as a repo of libraries that had no types. (read npm packages without types). It is not "the repo for types". The idea is that packages provide own types (authored by the package) and that TD / @types/etc slowly disappears

In the next version that has own types.. simply supply the types within the package.. anyone using TS will, after installing the package, have your types.. and will not need to install from @types/etc .. having to also install types from TD should be seen as a bad practice and can also introduce other problems (type packages with dependencies).

One thing you definitely do not want to do is keep a separate repo/branch with type definitions that need to be updated with each master source change.. I agree with @rcollette just move to TS (possibly with allowJS to get you going) and compile expected targets

rubiin commented 3 years ago

This is great. We can do the typescript version. I think the time consuming part is on the creating types @profnandaa

fedeci commented 3 years ago

On express-validator we have some incomplete type definitions that may work well as a starting point.

rubiin commented 3 years ago

On express-validator we have some incomplete type definitions that may work well as a starting point.

yeah that will help

vlascik commented 3 years ago

You already have a near complete rewrite at https://github.com/fireflysemantics/validatorts/ . Merge it here, replace that weird angular build system with tsc to create ambient declarations, use babel to generate es5/es6 etc. packages, and that's it. You can see an example of that e.g. here https://github.com/microsoft/TypeScript-Babel-Starter, but I'm sure more examples exist.

Every other solution is just pointlessly complicating things.

AllusiveBox commented 2 years ago

Curious if there was any decision actually made on how to go about using this? I am trying to argue for using this package instead of an internally managed one for work and they don't want to make the switch if there isn't any TypeScript support.

WikiRik commented 2 years ago

Curious if there was any decision actually made on how to go about using this? I am trying to argue for using this package instead of an internally managed one for work and they don't want to make the switch if there isn't any TypeScript support.

I don't believe so, but using @types/validator in combination with this package works fine in my projects

AllusiveBox commented 2 years ago

I don't believe so, but using @types/validator in combination with this package works fine in my projects

I missed that. Thanks, I'll let them know and see if that settles them for now.

tux-tn commented 2 years ago

I don't believe so, but using @types/validator in combination with this package works fine in my projects

I missed that. Thanks, I'll let them know and see if that settles them for now.

Agree with @WikiRik , @types/validator should cover all your needs.

adamgen commented 1 year ago

We can actually support types in a much better way.

DT offers type safety for the writing of the schema itself, however you cannot infer the end object's type from the schema like you can with other TS first libraries.

I've made a demo showcasing how we can easily infer types from our schema.

  1. Github repo
  2. Stackblitz link

@profnandaa I'd like to hear your thoughts.

braaar commented 1 year ago

As a TypeScript enthusiast I would be happy to contribute to this. Writing types for stuff is very satisfying :)

profnandaa commented 1 year ago

@adamgen -- definitely I'm ready revisiting this topic once again. Let us make this month's release and then resume this.

/cc @pano9000 @tux-tn @ezkemboi @rubiin

rubiin commented 1 year ago

I will create a draft then anyone who would like to contribute can push changes . CC https://github.com/validatorjs/validator-deno for reference

WikiRik commented 1 year ago

I think that it might be better to open a PR here that changes the infrastructure so that both TypeScript and JavaScript (including possible .d.ts files) are supported and we can migrate from JavaScript to TypeScript incrementally

rubiin commented 1 year ago

@WikiRik maybe you can start a PR and I can add in the changes. How does that sound. I am happy with either way

fedeci commented 1 year ago

For me the most viable solution is to convert the entire library to use TS. It is safer because of typings, prevents having to define types separately and it is quicker to understand the code for new contributors. I honestly cannot see drawbacks in adopting it.

adamgen commented 1 year ago

I have to take a step back from what I've proposed since it doesn't fit with the current state of validator.js. However, I'll take a few steps forward down the road.

Going type-heavy on validator.js in its current form will bring little to no value.

That's because the validator.js returns mostly primitive types. And a complex type system with inference, as zod has, is only relevant for schema validation, not for primitives. This gap between primitive vs. schema validation brings me to my next point.

Why wouldn't validator.js validate schemas? It already does on express-validator, where @gustavohenke has done a great job.

Now I'm entirely out of my domain with what I'm about to suggest, but this is how it goes.

I suggest having the schema validation already done by @gustavohenke in express-validator to be merged/combined with validator.js. Only then adding the inferred types I proposed earlier.

It'll be freaking awesome if you ask me. It'll give not less than scores of developers a lot of validation*type power without installing additional dependencies or migrating to new code, which I've found myself doing on a large project that maintained.

Also, I can be completely non-breaking, and since 80% of the work is done across the board, it requires a low time investment.

The 2 points above make a good open source ROI.

Is it too crazy? Is opening a new ticket for it will be a waste of time? Is there no PR that can move the needle forward on this topic?

ffxsam commented 1 year ago

I have one request if validator undergoes a major update:

Please simplify ESM tree-shaken imports:

import { isEmail, isURL } from 'validator';

validator/es/lib/isEmail is a bit inconvenient and clunky. The syntax I suggested will bring this package up to modern standards.

ST-DDT commented 1 year ago

@profnandaa / maintainers: What are your requirements for a community powered TS migration?

I assume that there are quite a few community members (in addition to me) that would help with the migration itself. We just need the requirements and an "go ahead".

WikiRik commented 1 year ago

@ST-DDT indeed minimal breaking changes, but I want a major release either way when we ship our own types so it's clear to the users. I'm willing to update types if needed after we have merged a PR that copies the .d.ts files of @types/validator. And then preferably slowly migrate from .d.ts/.js files in the source code to .ts files

ST-DDT commented 1 year ago

but I want a major release either way when we ship our own types so it's clear to the users

I understand that and it sounds reasonable.

I'm willing to update types if needed after we have merged a PR that copies the .d.ts files of @types/validator.

Is there a good time to start the process (as an external contributor) or do you have your own roadmap for that change?

WikiRik commented 1 year ago

I'm willing to update types if needed after we have merged a PR that copies the .d.ts files of @types/validator.

Is there a good time to start the process (as an external contributor) or do you have your own roadmap for that change?

As far as I know there is no roadmap, but it would be good to have approval from @profnandaa @tux-tn on a possible roadmap before we start executing it

rubiin commented 5 months ago

@profnandaa maybe its time to ship our own types file like most libs do https://github.com/validatorjs/validator.js/pull/2398

rubiin commented 5 months ago

I have one request if validator undergoes a major update:

Please simplify ESM tree-shaken imports:

import { isEmail, isURL } from 'validator';

validator/es/lib/isEmail is a bit inconvenient and clunky. The syntax I suggested will bring this package up to modern standards.

This might be a plan for v14. We maybe be opting to new modern tooling that provides a seemless transition from what we are using

profnandaa commented 5 months ago

Totally agree, will be good for v14, and maybe maintain a v13 branch and backport things for some time.

./na

On Thu, May 9, 2024, 10:20 Rubin Bhandari @.***> wrote:

I have one request if validator undergoes a major update:

Please simplify ESM tree-shaken imports:

import { isEmail, isURL } from 'validator';

validator/es/lib/isEmail is a bit inconvenient and clunky. The syntax I suggested will bring this package up to modern standards.

This might be a plan for v14. We maybe be opting to new modern tooling that provides a seemless transition from what we are using

— Reply to this email directly, view it on GitHub https://github.com/validatorjs/validator.js/issues/1271#issuecomment-2102101115 or unsubscribe https://github.com/notifications/unsubscribe-auth/AAB7ZEOWUMTSTSK2CRY2RQDZBMPVLBFKMF2HI4TJMJ2XIZLTS2BKK5TBNR2WLJDUOJ2WLJDOMFWWLO3UNBZGKYLEL5YGC4TUNFRWS4DBNZ2F6YLDORUXM2LUPGBKK5TBNR2WLJDUOJ2WLJDOMFWWLLTXMF2GG2C7MFRXI2LWNF2HTAVFOZQWY5LFUVUXG43VMWSG4YLNMWVXI2DSMVQWIX3UPFYGLAVFOZQWY5LFVIYTANRWHE4DIOBVHGSG4YLNMWUWQYLTL5WGCYTFNSBKK5TBNR2WLKRRGA3DMOJYHAZTENFENZQW2ZNJNBQXGX3MMFRGK3ECUV3GC3DVMWVDCMZRHE3DEMJXGA32I3TBNVS2S2DBONPWYYLCMVWKY43VMJVGKY3UL52HS4DFVREXG43VMVBW63LNMVXHJJTUN5YGSY3TSWBKI5DZOBS2U4TFOBXXG2LUN5ZHTJLWMFWHKZNGHE3DKNZYGKBKI5DZOBS2K2LTON2WLJLWMFWHKZNJGU4DKNZYGM4DMNECUR2HS4DFUVWGCYTFNSSXMYLMOVS2UMJQGY3DSOBUHA2TTAVEOR4XAZNFNRQWEZLMUV3GC3DVMWVDCMBWGY4TQOBTGI2IFJDUPFYGLJLMMFRGK3FFOZQWY5LFVIYTGMJZGYZDCNZQG6TXI4TJM5TWK4VGMNZGKYLUMU . You are receiving this email because you authored the thread.

Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub .

pano9000 commented 2 months ago

after spending some time with TypeScript the last couple of months, and definitely feeling the benefits of using it (from a DX point of view -> from a user point of view, you don't care if JS or TS -> you care about "bug free" or buggy :-)).

I wonder, why the initial post states "So, a rewrite is out of question."

It shouldn't be too much work, honestly -and it would make development experience a lot better IMHO and less error prone. IMHO some of the bugs in isDate for example, would likely have been prevented if TypeScript was used.

This would of course not be something for v13, but a major change.

Only thing holding this back of course is the ton of open PRs

Just droppping my 2 cents here :-)