Skip to content

Removes esModuleInterop from tsd and adds star import for helmet #95

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

Merged
merged 2 commits into from
Sep 14, 2020

Conversation

fox1t
Copy link
Member

@fox1t fox1t commented Sep 14, 2020

Since this is a plugin consumed by final users if we use esModuleInterop: true (as was before this PR) we broke all builds for users that have it set to false. In fact esModuleInterop is always checked in the dependency tree by TS if skipLibCheck is not set to true: we need to be sure that our package works for both esModuleInterop configurations.

On the other hand, Helmet doesn't use the "infamous triplet" (it still uses the old export = syntax), so we need to import it using import * as helmet from 'helmet' as it is a more conservative approach.

Fixes: #94

This PR can't be merged before this fix is published: fastify/fastify#2555

Checklist

Sorry, something went wrong.

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@mcollina
Copy link
Member

feel free to release

@fox1t fox1t merged commit 94ab6e0 into fastify:master Sep 14, 2020
@gtolarc
Copy link

gtolarc commented Sep 15, 2020

This patch seems to cause a problem with the esModuleInterop=true setting. Is there a solution?
스크린샷 2020-09-15 오전 11 00 50

@gtolarc
Copy link

gtolarc commented Sep 15, 2020

I think it would be better to revert. import * as helmet from "helmet"; -> import helmet from "helmet";

@mcollina
Copy link
Member

Can you send a PR?

@fox1t
Copy link
Member Author

fox1t commented Sep 15, 2020

Unfortunately, we can't just revert this because we will break the other case again. Can you create a repro repository so we can investigate further into this?

I can't understand why this worked both during my local testing and in the CI/CD.

I'll double-check the last import that is left to test:import helmet = require('helmet')

@fox1t
Copy link
Member Author

fox1t commented Sep 15, 2020

@mcollina here the fix: #96

I think that from now on if we need external types that are defined as export = (so CommonJS namespace) we should enforce the use of import = require. I'll check that this guideline is followed. In addition, as I propose in that PR, we need to add more tests to catch these scenarios.

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.

Error building project after Updating fasitfy-helmet to latest version 5.0.1
3 participants