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

[Question] How can we better support TypeScript? #1261

Closed
fireflysemantics opened this issue Feb 28, 2020 · 29 comments
Closed

[Question] How can we better support TypeScript? #1261

fireflysemantics opened this issue Feb 28, 2020 · 29 comments

Comments

@fireflysemantics
Copy link

Trying to use @types/validator and getting errors. I asked the question on SO here. Curious if anyone else is having the same issue?

https://stackoverflow.com/questions/60457757/typescript-imports-for-valiatorjs-failing

@fireflysemantics
Copy link
Author

This works:

import validator from "validator"

But that imports all of validator. In version 11 these types of imports used to work:

import { isDivisibleBy } from "validator";

I assumed that this was tree shakeable. I'm using validator in this project:

https://www.npmjs.com/package/@fireflysemantics/is

And each function is a typescript export. If validator is imported as a whole, then all the exported functions will include all of validator. It would be great if they would only include the specific function, but that seems to be enabled for ES6 only ... not typescript ....?

@profnandaa
Copy link
Member

I think @types has not been updated with the latest validator.js. I don't know how to go about it, perhaps you might have some idea? This must have been the last PR - DefinitelyTyped/DefinitelyTyped#40491

There's another requests to add our own typing here, perhaps as the primary source of truth; and then mirror it on _DefinitelyTyped`, see conversation in #1194 #789

/cc. @johannesschobel

@johannesschobel
Copy link
Contributor

If you offer own typings, you don't need to add them to DT anymore.

@fireflysemantics
Copy link
Author

Hmmm ... I upgraded this library to the latest version of validator using the import I described:

https://www.npmjs.com/package/@fireflysemantics/is

But now I'm getting errors:

node_modules/@fireflysemantics/is/target/is.d.ts:571:62 - error TS2503: Cannot find namespace 'validator'.

So I'm thinking the easiest thing to do is just rewrite the library in typescript and offer a validator-ts package.

Thoughts?

@profnandaa
Copy link
Member

@fireflysemantics -- I wouldn't call that the easiest option though...
@johannesschobel -- it looks to me that most libraries have stuck to using DT as a way of supporting TS types, a good example being lodash. I'd generally advocate for having the lib stay on JS, and have TS types supported separately.

@fireflysemantics
Copy link
Author

fireflysemantics commented Mar 1, 2020

I think I'm going to have a go rewriting the whole thing in Angular Package Format.

https://docs.google.com/document/d/1CZC2rcpxffTDfRDs6p1cfbmKNLA6x5O-NtkJglDaBVs/mobilebasic

This will compile support in for all the popular package formats and provide typescript typing. Did a small write up here:

https://medium.com/@ole.ersoy/creating-an-angular-9-npm-library-1a658ecfa3dc
https://medium.com/@ole.ersoy/unit-testing-your-angular-library-project-with-jest-42429a8716eb

When TS types are supported separately then Typescript is always playing catch up and as a result upstream builds break.

@fireflysemantics
Copy link
Author

fireflysemantics commented Mar 1, 2020

Here's what the toDate function would look like in Typescript with default exports removed (Because - https://blog.neufund.org/why-we-have-banned-default-exports-and-you-should-do-the-same-d51fdc2cf2ad):

import assertString from './util/assertString';

export function toDate(date:string) {
  assertString(date);
  const millis:number = Date.parse(date);
  return !isNaN(millis) ? new Date(date) : null;
}

This would be exported from the public surface (public-api.ts) like this:

export * from './lib/toDate';

And import goes to:

import { toDate } from './toDate';

from:

import toDate  from './toDate';

IIUC this should produce tree shakeable utility methods.

@fireflysemantics
Copy link
Author

#1263

@johannesschobel
Copy link
Contributor

@profnandaa , i wouldn't say, that it is the "go to strategy" to use DT. The major issue with DT is, that it is a separate repostiory where you need to publish your changes. Often, the library repository is managed by the core-developer team, and the DT repository is maintained by the community. So the "DT interface" often differs / does not reflect the current state of the library itself.

I mean - the cleanest (!) solution would be to re-write the entire library in TS and automatically build the JS version from there (i.e., use a GitHub action to build the JS version after every push to master). However, this would be a huge workload 😢

@profnandaa profnandaa changed the title [Question] Typescript imports are failing? [Question] How can we better support TypeScript? Mar 2, 2020
@profnandaa
Copy link
Member

Renaming this issue, and converging all TS conversations to this thread.

Calling on #789 #247 #662 #1194

@profnandaa
Copy link
Member

profnandaa commented Mar 2, 2020

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:

  • protect a small footprint for the library.
  • be less disruptive with what we already have.
  • keep supporting those who already were using DT (@types/validator).

Let me know what you folks think.

@johannesschobel
Copy link
Contributor

From the practical point of view, I think the only viable solution is 1

@fireflysemantics
Copy link
Author

fireflysemantics commented Mar 3, 2020

I'm almost done with the rewrite. I'll push it to this repository when it's complete:

https://github.com/fireflysemantics/validatorts

I'm hoping you will accept the changes and fork it back.

This is what I have done:

Angular Package Format

This is what I used. It means that all the popular package formats will be supported right out of the box (UMD, ES6, commonsjs, etc.) with each compile. The Angular CLI takes care of it.

It also means we get a simple flat tree shakeable API surface for all the validators and sanitizers that is consistent across all module formats, and that best practices are followed by default.

This improves the usability and simplicity of ValidatorJS.

TypeDoc

Added Typedoc documentation so that the the API is easily publishable in the Github Pages of the build

Named Exports Only

This means better tree shaking and refactoring for clients of the library. See this article:

https://blog.neufund.org/why-we-have-banned-default-exports-and-you-should-do-the-same-d51fdc2cf2ad

Jest for Testing

Each validator will have a validator.spec.ts file right next to it. This makes it very easy to see how to use the API for each validator from a unit testing perspective.

Pure Typescript Implementation

So that the types are generated for free and the library is always up to date. There is 0 chance of errors or other issues like the ones I'm now dealing with popping up, because the Typescript implementation is the yin and yang.

Separate Module Folders for Sanitizers and Validators

So that it's easier to see what is what. So we have:

  • Util
  • Sanitizers
  • Validators

Consistent Parameter Names

  • Target: The target of the validation
  • Arg: The parameter being tested against
/**
 * Compares the `target` and `arg` strings for strict (`===`) equality
 * 
 * @param target The target string to compare to
 * @param arg The argument that might equal the target 
 * @return true if the `target` is strictly equal to the `arg`
 */
export function equals(target: string, arg: string) {
  assertString(target)
  return target === arg
}

Nice to Have

I think the outdated Typescript @types definitions has typed the options arguments for some of the validators. It would be nice to inline these with the corresponding typescript implementations.

Inline Typedoc examples

In @fireflysemantics/slice I inlined API examples like this:

https://fireflysemantics.github.io/slice/doc/classes/_src_estore_.estore.html

This would be nice to have for validator also. I won't do this in the initial release, but I think it would be helpful for the community as a whole. The isIP code has a comment like this in the code, but it's not exposed via API documentation.

Example:

/**
 * Check if `target` is a valid port number
 *
 * @param target The port number
 * @return true if the `target` is a port number, false otherwise
 * 
 * @example

const isPortNumber:boolean = isPort('4200')
 */
export function isPort(target:string) {
  return isInt(target, { min: 0, max: 65535 });
}

Explicit Integer conversions

Some parts of the code reference arrays like this:

parts[1]

Converting these explicitly like this:

parseInt(parts[1])

Conversion Notes:

https://stackoverflow.com/questions/60510802/converting-validatorjs-isisin-to-typescript-and-getting-error/60511456#60511456

@fireflysemantics
Copy link
Author

Another thing I noticed is that Typescript "Caught" some things that I think are errors. I've asked about them in other implementation related issues for isIP etc. Things like 4 and 6 should be strings within the implementation I believe. Typescript at least thought so ...

@profnandaa
Copy link
Member

@fireflysemantics -- please check previous conversation above and respond.

@fireflysemantics
Copy link
Author

fireflysemantics commented Mar 3, 2020

@profnandaa my response is outlined above, I'll do the initial push hopefully today or tomorrow, and ya'll can see what you think. At a minimum Typescript users will have a consistent source to go to, while also gaining better tree shaking, the same consistent import set, and better refactoring since names will always be consistent.

@fireflysemantics
Copy link
Author

fireflysemantics commented Mar 4, 2020

I pushed the files:

https://github.com/fireflysemantics/validatorts

This is the Angular Library Package Format Section of the project:

https://github.com/fireflysemantics/validatorts/tree/master/projects/validatorjs/src

I'll deploy to verdaccio now and test out imports. When all is well I'll also add it to NPM.

Clone and build with the Angular CLI using ng build validatorjs

@fireflysemantics
Copy link
Author

This is what the package.json that is built using ng build validatorjs:

{
  "name": "@fireflysemantics/validatorts",
  "description": "String validation and sanitization",
  "version": "12.2.1",
  "homepage": "https://github.com/fireflysemantics/validatorts",
  "keywords": [
    "validator",
    "validation",
    "validate",
    "sanitization",
    "sanitize",
    "sanitisation",
    "sanitise",
    "assert"
  ],
  "bugs": {
    "url": "https://github.com/fireflysemantics/validatorts/issues"
  },
  "repository": {
    "type": "git",
    "url": "https://github.com/chriso/validator.js.git"
  },
  "license": "MIT",
  "comments": {
    "global dependencies": {
      "@jsdevtools/version-bump-prompt global install": "npm install -g @jsdevtools/version-bump-prompt",
      "npm-check-updates": "npm install -g npm-check-updates"
    },
    "validatorjs bugs": {
      "url": "https://github.com/chriso/validator.js/issues"
    },
    "validatorjs repository": {
      "type": "git",
      "url": "https://github.com/chriso/validator.js.git"
    }
  },
  "main": "bundles/fireflysemantics-validatorts.umd.js",
  "module": "fesm5/fireflysemantics-validatorts.js",
  "es2015": "fesm2015/fireflysemantics-validatorts.js",
  "esm5": "esm5/fireflysemantics-validatorts.js",
  "esm2015": "esm2015/fireflysemantics-validatorts.js",
  "fesm5": "fesm5/fireflysemantics-validatorts.js",
  "fesm2015": "fesm2015/fireflysemantics-validatorts.js",
  "typings": "fireflysemantics-validatorts.d.ts",
  "metadata": "fireflysemantics-validatorts.metadata.json",
  "sideEffects": false,
  "peerDependencies": {
    "tslib": "^1.10.0"
  }
}

Notice the module formats that are now generated for free, in addition to typescript definitions.

Here's a stackblitz demo:

https://stackblitz.com/edit/typescript-validatorts-demo?file=index.ts

Here's the NPM version:
https://www.npmjs.com/package/@fireflysemantics/validatorts

ValidatorJS now has:

  • Support for all popular module formats
  • First class support for tree shaking
  • Better refactoring due to named exports only
  • Automatically generated type definitions
  • A flat API surface across all module formats
  • Typedoc

And once the tests are translated, easier access to per validator unit tests with Jest.

@johannesschobel
Copy link
Contributor

Why do you use angular and not "pure" ts? Angular will add a lot of dependencies for this project that may not be needed at all!

@fireflysemantics
Copy link
Author

fireflysemantics commented Mar 4, 2020

@johannesschobel The build is using the Angular Package Format. Look at the package.json. There's only on peer dependency on "tslib": "^1.10.0".

The benefit of using the Angular Package Format and the Angular CLI for the build is that we get all the module formats built for us for free, in addition to the type definitions. The rest is the same.

If we use pure TS, we will not get all the package formats (UMD, etc.) built automatically.

@tux-tn
Copy link
Member

tux-tn commented Mar 4, 2020

Hello,
Concerning the current conversation, i think having a separate repo to handle TS type would be the easiest way to go.
I am trying to understand the need behind the rewrite you are doing @fireflysemantics . Switching software paradigm in a project can't be done without a practical and explicit need (Of course, you are free to fork and do your rewrite since it's an OSS project).
Your main issue was tree shaking in Typescript but you can accomplish it with es6 module import import isEmail from 'validator/es/isEmail'.

@fireflysemantics
Copy link
Author

fireflysemantics commented Mar 4, 2020

@tux-tn The rewrite is done and there are lots of other benefits, including now having the same paths for all imports regardless of whether you are doing tree shaking or not.

For example this has three issues:

import isEmail from 'validator/es/isEmail'

Default Imports

One is it used default imports so developers don't have to call the isEmail validator isEmail. They can call it isE or isEmailV or anything they like in their respective module. This breaks the ability to refactor the application later on a large scale.

Longer Import Path

The second is that when you want to have a more efficient application with tree shaking you have to use a longer import path.

Communication and Project Management

So some developers on your team might use that and some might not. And now you added an additional communication requirement around that, in addition to having to document the different paths and different formats.

Lag in Typescript Support

And finally if you keep the Typescript paths separate then there's always lag with respect to version updates in ValidatorJS or the Typescript types, which is why I did the conversion.

More Core Project Development Effort

Since Typescript is separate so is the task of keeping up with what has changed. It's additional effort for no real reason. There's hardly any difference between the Typescript implementation I did and the original current JS implementation. BUT there are a lot more benefits to the Typescript implementation.

Broken Typescript Support

Exporting single validator object with all the validators is a Tree Shaking Anti Pattern. Now all modules that use validator are pulling in all the validators in a single object.

Second when this project starting using this anti pattern it broke the current Typescript definition support and it's very easy, given the current circumstances, for this project to do so again in the future, since the Typescript definition types are not generated via the implementation.

@tux-tn
Copy link
Member

tux-tn commented Mar 4, 2020

One is it used default imports so developers don't have to call the isEmail validator isEmail. They can call it isE or isEmailV or anything they like in their respective module. This breaks the ability to refactor the application later on a large scale.

it's not a default import, you can have import isEmailAdress from 'validator/es/isEmail'

Longer Import Path

This was never a problem in the javascript ecosystem. If you think about it as a matter of code size, the difference is ridiculous and most people are using module bundlers.

Lag in Typescript Support

Broken Typescript Support

There's hardly any difference between the Typescript implementation I did and the original current JS implementation.

This is why i am saying your rewrite doesn't make sense and it will be better for the community to participate in the current conversation rather than try to make your own fork.

@fireflysemantics
Copy link
Author

It's fine. I respect that ya'll want to stick with what you have. I needed the conversion and will be adding more tests / typed interfaces for options / etc. and this will make the development experience much richer for me personally. Who knows maybe ya'll will come to like it as well one day :)

@tux-tn
Copy link
Member

tux-tn commented Mar 4, 2020

@fireflysemantics the beauty of open source is that you are free to do or use whatever you want. Good luck with your project 😃

@fireflysemantics
Copy link
Author

Added these types for isMobilePhone:

/**
 * Mobile Phone Local Type
 */
export type MobilePhoneLocale =
| 'am-AM'
| 'ar-AE'
| 'ar-BH'
| 'ar-DZ'
| 'ar-EG'
| 'ar-IQ'
| 'ar-JO'
| 'ar-KW'
| 'ar-SA'
| 'ar-SY'
| 'ar-TN'
| 'be-BY'
| 'bg-BG'
| 'bn-BD'
| 'cs-CZ'
| 'da-DK'
| 'de-DE'
| 'de-AT'
| 'el-GR'
| 'en-AU'
| 'en-GB'
| 'en-GG'
| 'en-GH'
| 'en-HK'
| 'en-IE'
| 'en-MO'
| 'en-IN'
| 'en-KE'
| 'en-MT'
| 'en-MU'
| 'en-NG'
| 'en-NZ'
| 'en-PK'
| 'en-RW'
| 'en-SG'
| 'en-TZ'
| 'en-UG'
| 'en-US'
| 'en-ZA'
| 'en-ZM'
| 'es-CL'
| 'es-ES'
| 'es-MX'
| 'es-PA'
| 'es-PY'
| 'es-UY'
| 'et-EE'
| 'fa-IR'
| 'fi-FI'
| 'fj-FJ'
| 'fo-FO'
| 'fr-FR'
| 'fr-GF'
| 'fr-GP'
| 'fr-MQ'
| 'fr-RE'
| 'he-IL'
| 'hu-HU'
| 'id-ID'
| 'it-IT'
| 'ja-JP'
| 'kk-KZ'
| 'kl-GL'
| 'ko-KR'
| 'lt-LT'
| 'ms-MY'
| 'nb-NO'
| 'nl-BE'
| 'ne-NP'
| 'nl-NL'
| 'nn-NO'
| 'pl-PL'
| 'pt-BR'
| 'pt-PT'
| 'ro-RO'
| 'ru-RU'
| 'sl-SI'
| 'sk-SK'
| 'sr-RS'
| 'sv-SE'
| 'th-TH'
| 'tr-TR'
| 'uk-UA'
| 'vi-VN'
| 'zh-CN'
| 'zh-TW'
| 'en-CA'
| 'fr-BE'
| 'zh-HK'

I believe the last three are missing regexes in the corresponding validator.js package:

https://github.com/fireflysemantics/validatorts/blob/master/projects/validatorts/src/lib/validators/isMobilePhone.ts

@profnandaa
Copy link
Member

@fireflysemantics -- looks like you met your objectives, I'll go ahead a close this issue, and open a fresh one to discuss viable TS options we would like to take for the library. Thanks!

@dandv
Copy link

dandv commented May 28, 2020

From an end-user's perspective, I don't see why "a TS rewrite is out of the question". I realize that from the developers' perspective that might well be the case, but as a user, I just want a clean TypeScript source that I can import without all the shenanigans like #1208, #1204, #1015 etc. It's 2020 after all.

@saeedhemmati
Copy link

It seems TS not implemented yet and still we need correctly type definition for better using of validator in ts projects

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

No branches or pull requests

6 participants