Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Integrate TS types into Helmet directly #188

Closed
elliotblackburn opened this issue Mar 7, 2019 · 11 comments
Closed

Integrate TS types into Helmet directly #188

elliotblackburn opened this issue Mar 7, 2019 · 11 comments
Assignees
Milestone

Comments

@elliotblackburn
Copy link

I noticed that for a while the helmetjs project has been active in reviewing updates to the DefinetelyTyped definitions for helmet and wondered if it was worth migrating the types to this repository.

They could still be a second class citizen, but it would mean users only need to include a single dependency to get the types and the helmet project would be able to ship changes more readily to the types without having to wait for the changes to go out via DT.

This may not be practical as it'll add overhead to the maintainership of helmet, but if time is already being spent to review the types it could actually reduce the effort even if changes are left to the community to submit PR's for.

As a user of helmet and typescript I'd be happy to help maintain the types and be tagged as a reviewer if in any community PR's.

This discussion has been bought over from DefinitelyTyped/DefinitelyTyped#33507

@EvanHahn
Copy link
Member

EvanHahn commented Mar 7, 2019

This is something I've thought about a fair bit.

While TypeScript support has certainly been on my mind, it hasn't felt like a pressing issue, so I've mostly kicked the can down the road. But given that TypeScript's adoption seems to be growing, the issue will only become more urgent.

Before talking about the options, it's important to me that I'm able to do some typechecking at runtime for anyone using JavaScript. For example, if you pass a number when you should've passed a string, I want to throw a runtime error. I could be talked out of this, but I think it's valuable, especially for a security module.

I think there are four possible options for Helmet:

  1. Helmet doesn't contain an ounce of TypeScript.
    • Pros: no problems with type checking at runtime; Helmet can typecheck "raw" JavaScript this way. No build steps.
    • Cons: a worse experience for TypeScript + Helmet users overall. In addition to having to install @types/helmet, users (1) have to install all of Helmet to get types for its sub-packages (2) might have mismatched versions.
  2. Helmet is written in JavaScript but has TypeScript definitions.
    • Pros: Helmet can still typecheck "raw" JavaScript at runtime. No build steps.
    • Cons: Tests would be duplicated (to be fair, this is how DefinitelyTyped works today, so this wouldn't be worse).
  3. Helmet is written in TypeScript.
    • Pros: One codebase. Additional typechecking in internal code.
    • Cons: We need a build step. We'd need to write JavaScript tests to check the at-runtime typechecking, so we'd need two test harnesses.

Based on this, I'm leaning towards Option #2. What do you (or others) think?

If we do any conversion, we should convert one sub-package at a time (ienoopen is probably the simplest).

@sandersn
Copy link

sandersn commented Mar 7, 2019

Hi, I'm the Definitely Typed maintainer (and on the Typescript team).

A few comments:

  • A solution to (1)'s sub-package problem might be to break @types/helmet into subpackages.
  • (1) and (2) both have the problem that the types don't necessarily relate to the actual source code. They are only checked against the type tests.
  • A drawback to (2) that we've observed is that non-TS projects have types that start off wrong and then fall out-of-date when the person who signed up to maintain them can't do it any more. With (2), you could require PRs to change the type tests whenever changing the actual tests. But then...
  • (2) and (3) both require contributors to know Typescript.
  • I don't understand why you would need two test harnesses for runtime typechecking in TS. I think runtime typechecking is a fairly common thing in Typescript as well.

Usually, I wouldn't recommend (2). It requires a lot of commitment from a typescript expert, or a little commitment and knowledge from everybody who contributes to the project -- about the same as (3), except that you don't notice when the types are wrong.

@EvanHahn
Copy link
Member

EvanHahn commented Mar 8, 2019 via email

@elliotblackburn
Copy link
Author

In the case of Helmet (and sub projects) where the api's are quite minimal I'm not sure a huge impact would be felt by having to update the types although it does add a barrier to contributors who don't know typescript as mentioned.

The question about testing for runtime type checks is interesting, I can't think of a way to do it without having some tests in raw JS because you'd be directly conflicting with the type system unless we just ignored the errors / warnings around those tests.

@sandersn
Copy link

sandersn commented Mar 8, 2019

@EvanHahn re runtime type checking and testing.

The testing is easy, you just cast the badly typed arguments to any:

() => {
  expect(() => f(1 as any)).toThrow("Expected string!")
}

I haven't done any runtime type checking myself since I'm too heavily steeped in static typing, but my co-workers have looked at io-ts (I found an introduction here.). I think the basic idea is that you still have to write some code to emit runtime type checks, but at least those checks are understood by the compiler while you're editing. I can't personally vouch for the library, but it's the name I've heard the most often in the Typescript world.

@EvanHahn
Copy link
Member

EvanHahn commented Mar 8, 2019

Just attempted to rewrite ienoopen in TypeScript. I'd appreciate some feedback: helmetjs/ienoopen#1

I'm familiar with TypeScript but am hardly an expert (notably when it comes to best practices, like which modules to install or how to structure projects).

@EvanHahn
Copy link
Member

ienoopen has been released with TypeScript support! We'll need to convert all other sub-packages.

Created a project for this.

@mroderick
Copy link
Contributor

I might be late to the party, but I think there's a fourth option missing from the list:

Use JSDoc to define types for everyone to benefit from:

  • Pros:
    • All (public) methods are documented
    • All the existing tooling for JavaScript still works
    • Autocomplete in VSCode 1, 2 (possibly others, but do TypeScript authors use other editors?)
    • One test harness
    • No build step
    • No dependency management of a compiler
  • Cons:
    • No build time warnings for helmet authors
    • The TypeScript artefacts still need to be generated, see potential solutions below

Potential solutions to generating the type definitions:

The approach suggested above is currently being investigated in the sinon organisation. In any case, we want to have API documentation, which we can generate from JSDoc. If we can use that effort to also give TypeScript authors a better experience, then that's a nice bonus 🍰

Converting helmet and its modules to TypeScript in order to give type definitions to TypeScript authors seem a bit overkill to me. Unless, of course @EvanHahn also enjoys writing TypeScript and find it beneficial to the work done in this repository.

@EvanHahn
Copy link
Member

@mroderick I like this JSDoc approach, but most of Helmet's subpackages (actually, maybe all? I need to check) have already been rewritten in TypeScript.

I'd be very curious to know what the sinon org does, as (1) I have a lot of respect for the package (2) I'm comfortable with the TypeScript setup for Helmet but am not in love with it, so I'd love to see other options for improvement.

@mroderick
Copy link
Contributor

I'd be very curious to know what the sinon org does, as (1) I have a lot of respect for the package (2) I'm comfortable with the TypeScript setup for Helmet but am not in love with it, so I'd love to see other options for improvement.

sinonjs/sinon#2003 is likely to be a good thread to follow regarding JSDoc. I imagine we'll leave breadcrumbs to whatever we end doing (or not) for TypeScript support

@EvanHahn EvanHahn self-assigned this Jun 12, 2020
@EvanHahn EvanHahn added this to the 4.0.0 milestone Jun 12, 2020
@EvanHahn EvanHahn mentioned this issue Jun 12, 2020
36 tasks
@EvanHahn
Copy link
Member

EvanHahn commented Aug 2, 2020

TypeScript types were added to Helmet in version 4, which you can install with npm install helmet@4. Closing this issue because I believe this is resolved, but feel free to comment or open a new issue.

@EvanHahn EvanHahn closed this as completed Aug 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

4 participants