search

ChrisCavs / aimless.js

The missing JavaScript randomness library.
https://chriscavs.github.io/aimless-demo/
MIT License
824 stars 13 forks source link

add type support #4

Closed ChrisCavs closed 1 year ago

chrskerr commented 1 year ago

Happy to help out with this if you'd like :)

here is the module I'm declaring for a personal project, but would be glad to help with something like writing in TS to export the types automatically 😃

declare module 'aimless.js' {
    export function seedFunc(seed: number): () => number;
    export function sequence<T>(array: [T, ...T[]], engine?: () => number): [T, ...T[]];
    export function oneOf<T>(array: [T, ...T[]], engine?: () => number): T;
}
kungfooman commented 1 year ago

here is the module I'm declaring for a personal project, but would be glad to help with something like writing in TS to export the types automatically smiley

So you are happy to rewrite everything in useless TS, making it impossible to use <script type="module"> while destroying the readability of pure JS code while ALSO requiring an extra build step...

TypeScript can generate .d.ts files from JavaScript with JSDoc aswell, so you don't even need to turn everything into ugly type interspersed word salad.

So the proper way is simply JS with JSDoc: KISS

jlarmstrongiv commented 1 year ago

@kungfooman Let’s break that down a little bit. I would assert that:

So you are happy to rewrite everything in useless TS

TypeScript, with over 30 million downloads per week, is not useless. I would encourage you to read of the companies and open-source libraries on this page that migrated to TypeScript.

destroying the readability of pure JS code

TypeScript code readability compared to JSDoc is subjective at best. I personally find that JSDoc typings are more verbose and less documented. Developers that I have worked with are more familiar with TypeScript too.

ALSO requiring an extra build step

We already need a build step to support multiple output formats—ESM and UMD. Plus, a build step is needed to polyfill older browsers.

impossible to use <script type="module">

TypeScript can be compiled to ESM, CJS, and IIFE. Of which, ESM is designed to work with <script type="module">

While JSDoc can be used for type information, you will likely still need // @ts-check at the top of all your files and additional linter rules (such as eslint-plugin-jsdoc) to receive the benefits of static typing. That way, you will have errors if your code is missing JSDoc types, or if the JSDoc types don’t actually match your code.

Since the maintainer of this repository @ChrisCavs himself created this issue, I am confident that this library will end up typed. The only question is which implementation he prefers—TypeScript, JSDoc, or handcrafted types.

I think @chrskerr’s offer to help out is very kind. I hope that TypeScript is adopted, but having solid types would be a net benefit regardless.

kungfooman commented 1 year ago

"subjective at best" :joy:

It's like arguing that everyone should use the elevator because one person has two broken legs and sitting in a wheel chair... or a diver telling everyone to carry oxygen flasks while shopping in a super market. If you don't know what I mean, that's exactly the problem: your fanboy-ism is making you blind to reality.

The cherry on top is that most TypeScript developers are hopelessly lazy. As soon as a type becomes just a little bit more complicated, they mentally drop out because they can't handle the task (search for any and ts-ignore...).

Shall we test your TypeScript skills and collect some subjective-at-best test data? :wink:

Type this: const concat = (a = [], b = []) => [...a, ...b];

Pro-tip: don't get it wrong. Some people need help to see reality, so please bear with me and I'm sure we can get to some sort of consensus.

jlarmstrongiv commented 1 year ago

@kungfooman I’ve experienced and used all three methods of adding types to libraries. In order to be effective:

Of all three implementations, I have found TypeScript to be the best experience:

In my (subjective) personal experience, strong typing in TypeScript has:

It's like arguing that everyone should use the elevator because one person has two broken legs and sitting in a wheel chair... or a diver telling everyone to carry oxygen flasks while shopping in a super market. If you don't know what I mean, that's exactly the problem: your fanboy-ism is making you blind to reality.

While TypeScript may seem heavy-handed at times, I usually need to dive into more advanced types for library code. That’s what makes autocomplete like zod and others so powerful. I’d rather setup TypeScript at the beginning, so that I can avoid converting the whole library when it outgrows JSDoc or hand-crafted types. Starting off with a strong foundation can avoid technical debt in the future.

The cherry on top is that most TypeScript developers are hopelessly lazy. As soon as a type becomes just a little bit more complicated, they mentally drop out because they can't handle the task (search for any and ts-ignore...).

It’s important to remember that TypeScript is not, in fact, JavaScript. TypeScript and strongly typed languages in general have unique patterns that need to be learned. While adding types to the JavaScript runtime world is most common, a lot of the difficulty comes in understanding the TypeScript type world or combining the two worlds together. I encourage you and others to study more advanced TypeScript with:

These exercises will be helpful if you aren’t familiar with generics, mapped types, and other type-specific concepts.

Shall we test your TypeScript skills and collect some subjective-at-best test data? 😉

Type this: const concat = (a = [], b = []) => [...a, ...b];

Pro-tip: don't get it wrong.

Speaking of Type Challenges, implementing a Concat type is actually one of their easy challenges.

First, let’s look at the official TypeScript Array.concat method:

interface ConcatArray<T> {
    readonly length: number;
    readonly [n: number]: T;
    join(separator?: string): string;
    slice(start?: number, end?: number): T[];
}

interface Array<T> {
    /**
     * Combines two or more arrays.
     * This method returns a new array without modifying any existing arrays.
     * @param items Additional arrays and/or items to add to the end of the array.
     */
    concat(...items: ConcatArray<T>[]): T[];
    /**
     * Combines two or more arrays.
     * This method returns a new array without modifying any existing arrays.
     * @param items Additional arrays and/or items to add to the end of the array.
     */
    concat(...items: (T | ConcatArray<T>)[]): T[];
}

Next, let’s type your original JavaScript function:

const concat = (a = [], b = []) => [...a, ...b];

With TypeScript using generics:

function genericConcat<A extends unknown[], B extends unknown[]>(a: A, b: B): [...A, ...B] {
    return [...a, ...b]
}

We can even make a more complex generic that supports more specific types like [1, 2, ...string[]] instead of just (string | number)[]. Making the result mutable is optional, but I have included it for reference.

type RecursiveMutable<T> = {
    -readonly [K in keyof T]: RecursiveMutable<T[K]>;
}
function constConcat<const A extends readonly unknown[], const B extends readonly unknown[]>(a: A, b: B): RecursiveMutable<readonly [...A, ...B]> {
    return [...a, ...b]
}

I think JSDoc has the @template tag for generics, but I believe it is more limited. When I’m diving this deeply into types, I’d much rather use TypeScript. Feel free to implement both TypeScript examples or parts of this codebase in JSDoc.

You may say that the syntax is poor or the example is too complex, but it’s uncommon to use this level of TypeScript, especially in application code. As a library author, I find the very specific TypeScript types improve the developer experience.

so please bear with me and I'm sure we can get to some sort of consensus

Again, it’s ultimately up to @ChrisCavs to decide which of the three implementations (TypeScript, JSDoc, or handcrafted types) he prefers to end up with .d.ts files for type support.

If he chooses JSDoc, would you be willing to contribute and add JSDoc annotations in the same way chrskerr volunteered to help with a TypeScript transition? Will you put your money time where your mouth is?

kungfooman commented 1 year ago

While TypeScript may seem heavy-handed at times, I usually need to dive into more advanced types for library code. That’s what makes autocomplete like zod and others so powerful. I’d rather setup TypeScript at the beginning, so that I can avoid converting the whole library when it outgrows JSDoc or hand-crafted types. Starting off with a strong foundation can avoid technical debt in the future.

TypeScript IS the technical debt. You are actively hindering developers to run your code without installing and running an elevator first. Or to take the other metaphor, you are binding people to a wheelchair and call it decreased technical debt because no one can take the stairs any longer (and when it starts burning, everyone dies).

Also companies seek people with 5 years of experience in whatever is brand-new on the market. :+1:

You are hunting shadows. If you need advanced types, just add a superMegaAdvanced.d.ts.

Whatever you can do in TypeScript, you can do in JSDoc aswell.

And you didn't actually implement the given concat function; the default arguments influence the typing process, which you simplified (maybe because you already lost track of what you were doing while trying to type this one-liner in TypeScript - which by now is a three-liner).

Wanna try again - this simple example of an easy challenge?

jlarmstrongiv commented 1 year ago

And you didn't actually implement the given concat function; the default arguments influence the typing process, which you simplified

Ahh, I missed that since I was coding late last night when I should have gone to bed 😂 in hindsight, I think that was off-topic. I don’t think you’re actually curious about the answer, but one potential solution is function overrides, similar to the original TypeScript Array.concat definition.

@ChrisCavs I have created a PR to convert this library to typescript in https://github.com/ChrisCavs/aimless.js/pull/8, with additional changes in the description.

Let’s see which one you end up choosing 😄

ChrisCavs commented 1 year ago

Hey folks. Lot of heated discussion on this issue. Here are my thoughts:

I've always considered TypeScript to be the only real option when it comes to typing JavaScript. This is probably entirely due to my ignorance on the matter. However I've used TypeScript almost exclusively for work and felt most comfortable with it as an option. It also seems to be the option with the most flexibility (in typing) and the most features.

However, this project was never about flexing my expertise. It was about getting out of my comfort zone and creating something awesome that people could use.

I was curious about JSDoc and what it could do, so I've spent about 20 hours over the past week exploring what that might look like and how I could get to a typed result.

I just published a beta version to npm, which utilizes JSDoc and an updated rollup config to generate a aimless.d.ts file. I believe the idea behind this approach is to write JSDoc comments and then utilize TypeScript to infer the types and generate the files, then use rollup to package the emitted type folder into a single directory file.

I would like for y'all to test what I have by installing the beta version 1.0.3-beta.0, and let me know if this provides sufficient typing for the library. Feel free to post here with feedback! If this result is insufficient, I'm happy to go back to the drawing board and refactoring for TypeScript.

jlarmstrongiv commented 1 year ago

@ChrisCavs I cloned the kungfooman:jsdocDevToolsTesting PR and installed the repo, but received this error:

types/aimless.d.ts → dist/aimless.d.ts...
[!] RollupError: Could not resolve entry module "types/aimless.d.ts".

With a cursory glance, here are notes from my PR if you end up continuing the JSDoc route:

Here are additional notes that may be considered out-of-scope:

@ChrisCavs definitely your prerogative to choose JSDoc. I encourage you to jump in, ask questions, and set a direction early and often to avoid duplication of efforts.

Have a great weekend everyone! That’s all from me.

ChrisCavs commented 1 year ago

@jlarmstrongiv you should be able to install the beta version from npm, or just pull my latest main branch since it's the same code as the beta version. Are these notes applicable to kungfooman's branch or to my beta release?

jlarmstrongiv commented 1 year ago

@ChrisCavs the build error is from the kungfooman:jsdocDevToolsTesting. All notes are applicable to aimless.js@1.0.3-beta.0.

kungfooman commented 1 year ago

@jlarmstrongiv You are easily confused about what you are doing and then blaming me twice? I didn't touch the rollup config, the reason you got that error is the new rollup config:

https://github.com/ChrisCavs/aimless.js/blob/3b5a3ca0e5d226912951ed710379808f2f67c0b5/rollup.config.mjs#L39-L43

The reason why Chris didn't get this error is because he run npm run typegen first.

TLDR: rollup needs to run the typegen command first

Simple as this in package.json:

  "scripts": {
    <snip>
    "prebuild": "rm -rf dist types && npm run typegen",
    <snip>
  },

Ahh, I missed that since I was coding late last night when I should have gone to bed in hindsight, I think that was off-topic. I don’t think you’re actually curious about the answer, but one potential solution is function overrides, similar to the original TypeScript Array.concat definition.

No worries, everything is fine. I actually do care, otherwise I wouldn't invest time here. I realized very early on that you didn't have too much experience with JSDoc, so I just want to freshen up your perspective a bit.

The funny thing is I used TypeScript for years myself, diving into type programming and telling everyone the world would be a better place once they adopted TypeScript. Until I realized that I can't read my JavaScript any longer once I'm done with the type programming. :joy:

But yea, I'm sure this line alone makes you - at least internally - admit what I'm talking about:

function constConcat<const A extends readonly unknown[], const B extends readonly unknown[]>(a: A, b: B): RecursiveMutable<readonly [...A, ...B]> {

Such thoughts require a certain degree of maturation before they are generally accepted. :wink:

ChrisCavs commented 1 year ago

@jlarmstrongiv thanks for the feedback on the beta version! I apologize for not commenting earlier, I appreciate the effort you put in to the typescript PR.

regarding CJS support: I currently have rollup spitting out a UMD which should be compatible with CJS, AMD, and IIFE. I added a second file for ES definition to allow more flexibility outside of UMD.

I like @template. interesting that it is not in the JSDoc documentation. I see it in the typescript JSDoc documentation however. I assume the // @ts-check is required to use features like @template? EDIT: I've been able to leverage @template by setting my eslint-jsdoc settings to "typescript", so i'm assuming this is something typescript enables. nothing wrong with that since I need typescript to bundle my types into dist anyways. the generic types are definitely better than * aka any.

regarding right click > Go to Definition errors, could you provide a bit more info or maybe an example? I'm wondering if there's anything I can do about that. in my editor it seems to work fine. I wonder i the issue is that it tries to go to the dist definition by default?

ChrisCavs commented 1 year ago

Hey folks, an important update:

After having successfully typed the repo using JSDoc and figured out the bundling required via Rollup, I've decided that this isn't a good direction for the repo. I will be refactoring to TypeScript and Parcel and ditching many of the dependencies currently listed.

My reasoning is as follows: it will be hard to maintain the repo moving forward without modern build tools (Parcel). Modern build tools are built around TypeScript, and I believe it is the superior option for typing JS. There are just too many workarounds required to support JSDoc, and JSDoc requires TypeScript support to work in any meaningful way regardless.

Additionally, I do not want to support users importing src files directly into their repos. I believe this is a bad practice that moves counter to the direction of the entire JavaScript community. I will continue to support ES modules alongside CJS.

You can expect a patch in the next week (v1.0.3) containing the TypeScript refactor.

ChrisCavs commented 1 year ago

I lied, I had time to do this today. Closing since addressed in v1.0.3

jlarmstrongiv commented 1 year ago

@ChrisCavs no worries! I’m glad to see v1.0.3 supporting types. It seems you’ve added the type map too, which is a nice touch. I appreciate your reasoning throughout this issue.

@kungfooman not blaming you for the build error, I just thought I ran into https://github.com/ChrisCavs/aimless.js/pull/7#discussion_r1215671554 or something else entirely, like having the wrong version/branch. I decided to look into aimless.js@1.0.3-beta.0 instead of the error because I was short on time. It’d be good to put npm run typegen into a prebuild script.

Finally, @kungfooman, you are right that I am not as well-versed in JSDoc as you. I usually use TSDoc with Typescript. If the package is very small, then I sometimes author my code in .js and add .d.ts files, which are then checked with tsd or tsc.

Again, JSDoc, TypeScript, and Handcrafted Types are all valid solutions to the problem. I still prefer TypeScript due to the reasons above, but you are more than welcome to prefer JSDoc. Similarly to how several companies and libraries migrated to TypeScript, I do know that eslint may be sticking with JSDoc https://github.com/eslint/eslint/discussions/16557

While TypeScript can get messy, such as that line you quoted, I find that many more of those situations are when converting JavaScript to TypeScript, rather than starting with TypeScript at the beginning. And, in library code, I usually need to dive down into more TypeScript-specific syntax, so it’s easier to have TypeScript support at the get-go too. As a library author, I also try to make my types as specific as possible, as I think it improves the developer experience of the end user.

I am also probably biased due to the code bases I have worked with. The pure JavaScript ones have been much harder to work with, compared to the TypeScript ones. And one of the larger JSDoc codebases I have worked with did not use // @ts-check or eslint-plugin-jsdoc, which led to a lot of issues down the line—it was not possible to generate types from that code base, despite the JSDoc hints—plus other time sinks. Luckily, Aimless.js did not have nearly as many issues as that code base. At the end of the day, I have also spent significantly more time with TypeScript code bases, and have become much more familiar with its syntax and idiosyncrasies.

If JSDoc improves their documentation/resources and supports more type features, I’d be glad to take another look at it someday. For now though, I’ll be sticking with TypeScript, warts and all.

I think that tone in this asynchronous chat has also been difficult to get right, and I apologize for any remarks that came across poorly. At the end of the day, I’m just glad that this library now has solid types.