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

Issues with TypeScript imports in 4.0.0 #235

Closed
zen0wu opened this issue Aug 2, 2020 · 25 comments · Fixed by #245
Closed

Issues with TypeScript imports in 4.0.0 #235

zen0wu opened this issue Aug 2, 2020 · 25 comments · Fixed by #245

Comments

@zen0wu
Copy link

zen0wu commented Aug 2, 2020

Code like below overrides the exported object, which causes default being missing from every module.

module.exports = contentSecurityPolicy;
export default contentSecurityPolicy;

For example, inside index.ts, add console.log("wat", expectCt); under the import, and we'll see it's undefined.

@EvanHahn
Copy link
Member

EvanHahn commented Aug 2, 2020

What does your build setup look like? I didn't experience this problem when testing things, but I may have made a mistake.

Also: are you using helmet or helmet-csp?

@zen0wu
Copy link
Author

zen0wu commented Aug 3, 2020

I'm using helmet.

I believe this issue is universal. For example, create a test.ts in the root of helmet project with

import helmet from './index' 
// import * as helmet from './index' confused TS and caused type error

console.log(helmet)

It outputs undefined.

@EvanHahn
Copy link
Member

EvanHahn commented Aug 3, 2020

A couple of questions to help me debug:

  1. What does your tsconfig.json look like? Specifically, what is the value of esModuleInterop?
  2. Do you still have @types/helmet installed?

@hongbo-miao
Copy link

hongbo-miao commented Aug 3, 2020

Seems like a TypeScript config issue.

I just upgrades to helmet 4.0.0, the code is at https://github.com/Hongbo-Miao/hongbomiao.com/blob/master/server/src/security/middlewares/helmet.middleware.ts
tsconfig.json is at https://github.com/Hongbo-Miao/hongbomiao.com/blob/master/server/tsconfig.json

Hopefully it helps.

@anthony-telljohann
Copy link

I was able to resolve this by ignoring typescript errors before using helmet

import * as helmet from 'helmet'
...
// @ts-ignore
app.use(helmet(helmetConfig))

I believe this is because the default helmet export has an incorrectly defined type.

@EvanHahn EvanHahn changed the title New export behavior is broken in 4.0.0 Issues with TypeScript imports in 4.0.0 Aug 3, 2020
@EvanHahn
Copy link
Member

EvanHahn commented Aug 3, 2020

I'd like to fix the Helmet types.

Helmet is a CommonJS module (in other words, it uses module.exports and not export ...). Therefore, without esModuleInterop, I would expect users to do something like this:

import helmet = require("helmet");

With esModuleInterop, I'd expect something like this to work:

import helmet from "helmet";

I'm not an expert at how TypeScript modules work, nor am I certain why some people are having problems and some are not.

If anyone has suggestions for how to fix this, I'd appreciate it!

@tobeno
Copy link

tobeno commented Aug 5, 2020

To my understanding you would need to export helmet using export = helmet (similar to what was done in the old 3.x.x typings in https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/helmet/index.d.ts) in your index.ts file. This would replace both the default export and the module.exports. Otherwise import helmet = require('helmet') (or import * as helmet from 'helmet') will not work as the TypeScript compiler thinks there is only a default export.

This seems to be the correct way to handle CommonJS style exports in TypeScript: https://www.typescriptlang.org/docs/handbook/modules.html#export--and-import--require

As far as I understand the concept, this should also still work correctly if importing projects use esModuleInterop=true. So hopefully not a breaking change. But that should be tested in some dummy project before publishing it to NPM as a minor/patch version.

In the future it could be interesting to move away from exporting the function directly and use a named export instead (so import { helmet } from 'helmet' and const { helmet } = require('helmet'); ).

As an alternative you could also look into creating a separate ESM build with a proper default export. But that would of course break existing import * as helmet from 'helmet'. So in any case a breaking change would be needed for either solution.

Think for now sticking with pure CommonJS seems to be the best bet.

@sehrope
Copy link

sehrope commented Aug 6, 2020

+1 for named exports. They play nice with editor auto completion as well.

@EvanHahn
Copy link
Member

EvanHahn commented Aug 7, 2020

Is there a way to do export = helmet and still export the types, such as HelmetOptions? I think the answer is "no".

@thepostmillennial
Copy link

I got the same issue when I upgrade the helmet from 3.23.3 to 4.0 in my NestJS typescript project. any idea how to resolve this issue?

@zen0wu
Copy link
Author

zen0wu commented Aug 9, 2020

re named exports - They're generally a good idea IMO. Since this is a major version, it's a good reason to justify the change to named exports?

@EvanHahn
Copy link
Member

This should be fixed in the next version. Please try it out with npm install helmet@4.1.0-rc.2 and let me know what you think. Once I get a few successful reports, I'll release it.

@razorness
Copy link

import helmet = require('helmet');

// ...

app.use(helmet());

Tested with TypeScript v3.9.7 and NestJS:

$ nest info

 _   _             _      ___  _____  _____  _     _____
| \ | |           | |    |_  |/  ___|/  __ \| |   |_   _|
|  \| |  ___  ___ | |_     | |\ `--. | /  \/| |     | |
| . ` | / _ \/ __|| __|    | | `--. \| |    | |     | |
| |\  ||  __/\__ \| |_ /\__/ //\__/ /| \__/\| |_____| |_
\_| \_/ \___||___/ \__|\____/ \____/  \____/\_____/\___/


[System Information]
OS Version     : Windows 10
NodeJS Version : v12.18.2
YARN Version    : 1.22.4

[Nest CLI]
Nest CLI Version : 7.4.1

[Nest Platform Information]
platform-express version : 7.0.0
passport version         : 7.1.0
typeorm version          : 7.1.0
common version           : 7.0.0
core version             : 7.0.0

Looks good to me. Thanks!

@czzonet
Copy link

czzonet commented Aug 12, 2020

First thing is make it work, optimizing the type export is the next. :)

@EvanHahn
Copy link
Member

First thing is make it work, optimizing the type export is the next. :)

@czzonet What do you mean?

@czzonet
Copy link

czzonet commented Aug 12, 2020

It is better if it can work like this? @EvanHahn

import {helmet, HelmetOptions} from 'helmet'

@pietrzakadrian
Copy link

change the helmet version to this: "helmet": "4.1.0-rc.2", in package.json

@EvanHahn
Copy link
Member

@czzonet I don't think that's possible, unfortunately. It's arguably a breaking change, but it only affects TypeScript users where things weren't working well anyway.

@pietrzakadrian What do you mean? Is there something wrong with Helmet, or are those instructions for other Helmet users?

@anthony-telljohann
Copy link

@EvanHahn After installing helmet@4.1.0-rc.2, I was able to fix the typescript errors by importing the default export:

import helmet from 'helmet'
app.use(helmet(helmetConfig))

@sehrope
Copy link

sehrope commented Aug 14, 2020

+1 for helmet@4.1.0-rc.2. Typing issues are resolved.

For anybody that was previously importing the HelmetOptions interface, e.g. you're assigning the options to a variable rather than directly invoking the middleware function, you can do it by extracting the type from the middleware arg:

import helmet = require('helmet');
const opts: Parameters<typeof helmet>[0] = {
    // strongly typed helmet options go here...
};
helmet(opts);

@EvanHahn
Copy link
Member

Planning to release helmet@4.1.0 soon.

@EvanHahn
Copy link
Member

This has been fixed in helmet@4.1.0. Please let me know if you run into any issues!

@joshuajung
Copy link

Works like a charm. For everyone running into issues, make sure to remove any earlier installations of @types/helmet.

@jsaguet
Copy link

jsaguet commented Aug 18, 2020

You can also use

import helmet from 'helmet';
app.use(helmet());

if you use "allowSyntheticDefaultImports": true, in your tsconfig.json
(works with TS 3.9.7)

@fox1t
Copy link

fox1t commented Aug 18, 2020

In order to have the same behavior as before we had to import that interface like this: https://github.com/fastify/fastify-helmet/blob/6637767c0b3d088e0f8cf0b440112df4c48f9566/index.d.ts

Said that I have to say that we had many problems with esModuleInterop in the fastify ecosystem, but we fixed all of them and now we support seamlessly TS, CJS and ESM, all with correct typings. Our solution was the "infamous triplet".

You can read more about it here and here

If you want to follow the same approach, I'll be happy to help you, if needed/wanted.

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

Successfully merging a pull request may close this issue.