session icon indicating copy to clipboard operation
session copied to clipboard

Request for comments / direction: Add types into repo and type checking without converting to typescript

Open raphendyr opened this issue 9 months ago • 3 comments

This is my test to add types to the repo. I wanted to take care of the following

  1. Ensure types are close to the code with hope they stay updated and correct
  2. Types would be defined in a single place. That would be .d.ts file or JSDoc. Final .d.ts files could be generated when building the package.
  3. Ability to validate types. For the purpose to keep types correct, but also to provide static type checking to the implementation.
  4. Based on discussion, it seems full Typescript rewrite might not be wanted. Hence, I tried to use the second best technology JSDocs.

Based on this work. I find that:

  1. Using JSDoc for this purpose is complicated. It's amazing how well it works, but there are issues that limit the ability in some cases, here for the implements/extends CookieOptions.
  2. Google results, documentation and how Typescript works for JSDoc is very limited. I would predict that there are limited amount of people who would be able to work with this.
  3. Codebase has some cohesion problems and hacks, which I think would have been mitigated if type checking would have been here from the start. See Store.createSession, which used to workaround how Cookie implementation works. There are some error propagation issues too, which I'm not totally sure. Might be my own doing or I just triggered those. I commented out those test at this point.

If people see benefits for static type checking, then I think it would make more sense to use Typescript in the first place. EDIT: newer iteration shows that maybe mix of d.ts and JSDoc might be ok.

If people only want to include types in the repo, then it might be more suitable to define types only for the external API. This notably loses the type checking. In addition, I'm not sure how to design a type validation in that case or how to verify it's up-to-date.

What do you people think?

Do you find good things from the code?

Do you find something that makes you worry?

~~Note that I focused on a single file, the cookie.js. Moving forward would require more work, but I wanted to talk about this direction before spending any more time.~~ The cookie.js and session.js are now ok.

This PR is based on the previous work in #656

raphendyr avatar Mar 18 '25 19:03 raphendyr

Hey, thanks for opening this issue. This is a bit on hold for now since it requires some more discussion (see https://github.com/expressjs/typescript-wg/issues/1). I know having the types integrated is great, but it's not a top priority at the moment. However, I can review it soon since it's something I'm interested in.

bjohansebas avatar Apr 03 '25 21:04 bjohansebas

This is a bit on hold for now since it requires some more discussion (see expressjs/typescript-wg#1).

Thanks for reminding about that again. It's important discussion after all.

I know having the types integrated is great, but it's not a top priority at the moment.

I know. I tried to leave it after first iteration, but I really liked types when I was working on my idea to move the header handling code behind some interface and came back to types to support that rework.

And because it's not priority, I wanted to open it as a discussion place with some concrete examples.

However, I can review it soon since it's something I'm interested in.

I don't need it needs "review" with the goal to merge it, but feel free to read the changes and write your opinions and feelings freely. At this point, I think it's more valuable to have more opinions and development ideas. I (or someone) can then test those out. Maybe we finally get something that can be considered for merging.

Thank any how!

raphendyr avatar Apr 09 '25 09:04 raphendyr