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

Fix typescript issues after latest release 7.1.0 #94

Merged
merged 2 commits into from
Nov 27, 2024

Conversation

julienw
Copy link
Contributor

@julienw julienw commented Nov 20, 2024

With latest release 7.1.0, I had to install helmet as a dependency. This produced typescript errors (with all version of Helmet I tried):

type HelmetOptions = helmet.HelmetOptions;

gives

node_modules/koa-helmet/koa-helmet.d.ts:11:22 - error TS2503: Cannot find namespace 'helmet'.

11 type HelmetOptions = helmet.HelmetOptions;
                        ~~~~~~

This is fixed easily enough by importing HelmetOptions directly instead.

But then we get another error:

node_modules/koa-helmet/koa-helmet.d.ts:70:38 - error TS2339: Property 'expectCt' does not exist on type 'HelmetOptions'.

70     expectCt(options?: HelmetOptions['expectCt']): Middleware;

That's happening because helmet removes expectCt and as a result this isn't available anymore. Helmet folks suggest to use the expect-ct package if needed, but I'm reluctant to add this as another peer-dependency if most users of koa-helmet don't need it.

So instead I'm using a mapped type to construct koa-helmet's default export. This should adapt koa-helmet's type with whatever helmet version a project uses.

I'm also using the JS module syntax instead of the common module syntax.

I also had to fix new typescript issues in the test file that were surfaced after this change, which proves that the change is beneficial.
This is probably a breaking change and will need a major update, but given 7.1.0 should have been a major update too, I'd say it's OK.

Please tell me what you think!

@julienw
Copy link
Contributor Author

julienw commented Nov 26, 2024

gently ping @venables :-)

@venables
Copy link
Owner

Thanks @julienw

@venables venables merged commit 0820332 into venables:main Nov 27, 2024
3 checks passed
@venables
Copy link
Owner

This has been included in v8.0.0. I apologize for the breaking change in 7.1.0, which also should have been a major release.

@julienw
Copy link
Contributor Author

julienw commented Nov 28, 2024

Thanks so much for the merge and the quick release!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants