Request for comments / direction: Add types into repo and type checking without converting to typescript #1031
Conversation
|
New, updated, and removed dependencies detected. Learn more about Socket for GitHub ↗︎
|
| * @api public | ||
| */ | ||
|
|
||
| serialize: function(name, val){ |
There was a problem hiding this comment.
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.
| this.httpOnly = options.httpOnly ?? true | ||
| this.domain = options.domain | ||
| this.path = options.path || '/' | ||
| this.sameSite = options.sameSite |
There was a problem hiding this comment.
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.
| this.path = options.path || '/' | ||
| this.sameSite = options.sameSite | ||
|
|
||
| this.signed = options.signed // FIXME: how this is used?? |
There was a problem hiding this comment.
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
force-pushed
the
add-types-and-type-checking
branch
from
529c048 to
608f50a
Compare
There was a problem hiding this comment.
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.
|
|
||
| /** | ||
| * Return cookie data object. | ||
| * | ||
| * @return {Object} | ||
| * @api private | ||
| * @return {CookieSerializeOptions} |
There was a problem hiding this comment.
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.
This is my test to add types to the repo. I wanted to take care of the following
.d.tsfile or JSDoc. Final.d.tsfiles could be generated when building the package.Based on this work. I find that:
CookieOptions.Store.createSession, which used to workaround howCookieimplementation 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.
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.This PR is based on the previous work in #656