Psifi-Solutions / csrf-csrf

A utility package to help implement stateless CSRF protection using the Double Submit Cookie Pattern in express.
Other
123 stars 19 forks source link

refactor: move types into types.ts #47

Closed psibean closed 11 months ago

psibean commented 11 months ago

This PR should not have any API impact, it is purely a typescript refactor which maintains existing build output and behaviour.

I compared the new type declaration output to the old / previous and it appears to me to be the same - if someone else could confirm / verify that would be nice!

Since we have a breaking change to fix a current bug / missing feature, I also plan on renaming some of the types to be less generic to avoid potential type name clashing in the global scope (alternatively, wrap declared types in a "csrf-csrf" module block).

psibean commented 11 months ago

Looks pretty good to me, but the build command seems to leave an 'empty' types.js file (both on the esm and cjs builds), which shouldn't cause any problems but probably isn't intended. After looking for a while it seems it is the way tsc works. Also, if types are being explicitly declared in the types.ts file, shouldn't it be named types.d.ts or something similar?

The compilation will still output it as a declaration file. I don't want a manual declaration file to be maintained, the ts file will still have type checking on it where a hard coded declaration wouldn't.

The point of moving them is just to separate the code a bit - make it easier to maintain / review and move the type only things out of the implementation.

The second one here being the most important, it's a side effect of tsc trying to support certain build tools. Seems to be up in the air whether they'll provide an option to prevent export {} / empty module generation.

Lots of people complaining that type only exports / imports end up with this side effect.

Using tsup to also transpile the ts implementation seems to give a different output compared to tsc, so I don't think I want to make that switch yet, although it would prevent the empty output.

For now I just added some manual removals of the empty types.js