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

raphendyr · 2025-03-18T19:39:26Z

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

Ensure types are close to the code with hope they stay updated and correct
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.
Ability to validate types. For the purpose to keep types correct, but also to provide static type checking to the implementation.
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:

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.
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.
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

socket-security · 2025-03-18T19:39:41Z

New dependencies detected. Learn more about Socket for GitHub ↗︎

Package	New capabilities	Transitives	Size	Publisher
npm/@types/cookie-signature@1.1.2	None	`0`	3.42 kB	types
npm/@types/cookie@0.6.0	None	`0`	10.1 kB	types
npm/@types/debug@4.1.12	None	`+1`	11.6 kB	types
npm/@types/depd@1.1.37	None	`0`	5.67 kB	types
npm/@types/express@5.0.1	None	`+9`	98.4 kB
npm/@types/node@18.19.82	None	`+1`	2.13 MB	types
npm/@types/on-headers@1.0.3	None	`0`	4.28 kB	types
npm/@types/parseurl@1.3.3	None	`0`	3.16 kB	types
npm/@types/uid-safe@2.1.5	None	`0`	3.09 kB	types
npm/typescript@5.8.2	None	`0`	22.9 MB	typescript-bot

View full report↗︎

raphendyr · 2025-03-18T19:44:42Z

session/cookie.js

-   * @api public
-   */
-
-  serialize: function(name, val){


I don't know if this is used by external Store implementations. In this repo, this was used only by the test code. The version in index.js calls cookie.serialize directly on purpose or by accident.

First commit removed this method, but the second is more about just adding types and converting to a class, hence this method is back. The above comment still applies, and this method probably could be removed.

raphendyr · 2025-03-18T19:49:50Z

session/cookie.js

+      this.httpOnly = options.httpOnly ?? true
+      this.domain = options.domain
+      this.path = options.path || '/'
+      this.sameSite = options.sameSite


Using a for loop here is not good from the type checking point of view (the for loop doesn't understand what type keys are).

The Object.assign(this, options) does work here, but then we need to skip options.data property some how. For some reason, people have been passing it here and it fails as data is readonly for Cookie.

As a good result, this fixes the problem with data field, which was special case. In addition, the code is quite clean.

As a down side, now these properties are hardcoded in quite many places and changes to those require care.

For the second iteration, I used the old code, just added type overwrites, so that the for loop works. I think the outdated code is kind of better, but I prefer the less rewriting style at this point.

raphendyr · 2025-03-18T19:51:00Z

session/cookie.js

+      this.path = options.path || '/'
+      this.sameSite = options.sameSite
+
+      this.signed = options.signed // FIXME: how this is used??


For some reason there are undocumented properties in the type file. I'm didn't yet search what these were, why and for what purpose.

raphendyr · 2025-03-18T20:02:31Z

types/cookie.d.ts

Note, this file is generated by the tsc command (npm run check-types). Most notably, that doesn't copy cookie-options.d.ts over, thus this folder doesn't for packaging as is.

The type generation from JSDoc works quite well.

The JSDoc comments in this file are limited, but at least vscode can find the description part from the CookieOptions when hovering over Cookie properties. I'm not sure if JSDoc tool can do the same.

This is fixed by using something else than .d.ts as the file. I went with .types.ts suffix, but this could be anything I think.

raphendyr · 2025-03-18T20:03:10Z

session/cookie.js


  /**
   * Return cookie data object.
   *
-   * @return {Object}
-   * @api private
+   * @return {CookieSerializeOptions}


This interface was and is still used for multiple purposes. The code isn't very clear who it's serving.

Specifying this type, it makes it clear it has a single purpose. As a result, the property could be renamed.

For now, I used this for toJSON, but it might be better to separate these two.

Second iteration went back to the current API, which is a mix. As a result, it now uses object as type.

raphendyr · 2025-03-24T11:06:33Z

session/session.js

+   * @public
+   */
+  resetMaxAge() {
+    if (this.cookie) {


This is a "fixthat is result of a type checking. TheSession.cookie` can be undefined. If I remember right, there is issue about some edge case when this happens.

At least, this happens when there is a code bug in Session.constructor().

bjohansebas · 2025-04-03T21:49:01Z

Hey, thanks for opening this issue. This is a bit on hold for now since it requires some more discussion (see expressjs/discussions#192). 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.

raphendyr · 2025-04-09T09:04:36Z

This is a bit on hold for now since it requires some more discussion (see expressjs/discussions#192).

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 commented Mar 18, 2025

View reviewed changes

wip: look into adding type checking and types

608f50a

raphendyr force-pushed the add-types-and-type-checking branch from 529c048 to 608f50a Compare March 18, 2025 19:55

raphendyr commented Mar 18, 2025

View reviewed changes

wip: less rewriting way of adding types

46b55ae

raphendyr force-pushed the add-types-and-type-checking branch from 3750359 to 46b55ae Compare March 24, 2025 11:02

raphendyr commented Mar 24, 2025

View reviewed changes

raphendyr mentioned this pull request Apr 9, 2025

EFI: Publish and maintain types expressjs/discussions#192

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

raphendyr commented Mar 18, 2025 •

edited

Loading

socket-security bot commented Mar 18, 2025 •

edited

Loading

raphendyr Mar 18, 2025

raphendyr Mar 24, 2025

raphendyr Mar 18, 2025

raphendyr Mar 24, 2025

raphendyr Mar 18, 2025

raphendyr Mar 18, 2025

raphendyr Mar 24, 2025

raphendyr Mar 18, 2025

raphendyr Mar 24, 2025

raphendyr Mar 24, 2025

bjohansebas commented Apr 3, 2025

raphendyr commented Apr 9, 2025

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

Are you sure you want to change the base?

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

Conversation

raphendyr commented Mar 18, 2025 • edited Loading

socket-security bot commented Mar 18, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bjohansebas commented Apr 3, 2025

raphendyr commented Apr 9, 2025

raphendyr commented Mar 18, 2025 •

edited

Loading

socket-security bot commented Mar 18, 2025 •

edited

Loading