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

passport typescript type conflict #47

Closed
wyattjoh opened this issue Jul 9, 2018 · 6 comments
Closed

passport typescript type conflict #47

wyattjoh opened this issue Jul 9, 2018 · 6 comments

Comments

@wyattjoh
Copy link

wyattjoh commented Jul 9, 2018

Trying to use this package along with a custom passport strategy with express.

Issue is that this package includes the definitions from express-jwt, which are causing issues with projects that include both passport and node-jwks-rsa.

DefinitelyTyped/DefinitelyTyped#23976

Any recommendations to resolve this conflict of the Request.user type?

node_modules/@types/express-jwt/index.d.ts(43,13): error TS2717: Subsequent property declarations must have the same type.  Property 'user' must be of type 'User | undefined', but here has type 'any'.
@joshcanhelp
Copy link
Contributor

@wyattjoh - Thanks for the report here! We're taking a look internally and post back when we have a path forward 👍

@joshcanhelp
Copy link
Contributor

@wyattjoh - I wanted to check back in with you here ... it looks like the main culprit here is a change made to the Passport definitions earlier this year (commit). That switched it from any to User, which collides with the any directive in Express JWT. I'm going to put through a PR to DefinitelyTyped to hopefully correct this but would like to be able to test it out first before doing that.

Can you provide some simple repro steps to generate this error? If your project is open source and has install instructions, that would be fine. Otherwise, just a way that I can generate that error, then test with the fix in place.

Thank you!

@wyattjoh
Copy link
Author

Sure @joshcanhelp!

You can grab the following repo/branch: coralproject/talk#1743

git clone https://github.com/coralproject/talk.git
cd talk
git checkout next-passport
npm install
npm run compile:graphql
npm run build:server

That currently, will throw the following error:

node_modules/@types/express-jwt/index.d.ts(43,13): error TS2717: Subsequent property declarations must have the same type.  Property 'user' must be of type 'User | undefined', but here has type 'any'.

@joshcanhelp
Copy link
Contributor

Thanks for that, works as expected ... AFAIK :)

And just so I'm sure, I'm expecting no errors and a /dist/ folder created with compiled JS? If so, that's what I get with my proposed fix for this:

// node_modules/@types/passport/index.d.ts:15

declare global {
    namespace Express {
        interface Request {
            authInfo?: any;
            user?: User | any; // still use the `User` interface but allow `any` as well
            // ...
        }
    }
}

If you can just confirm what I'm getting or make that change and try it out, I'll put through a PR for that.

@wyattjoh
Copy link
Author

That looks like it works perfectly @joshcanhelp! Would love a PR submitted to resolve it 😄

@joshcanhelp
Copy link
Contributor

joshcanhelp commented Jul 16, 2018

PR submitted: DefinitelyTyped/DefinitelyTyped#27339

prmtl referenced this issue in DefinitelyTyped/DefinitelyTyped Sep 13, 2019
* Fix up Request.authInfo/user declarations in Passport

* Add myself

* Use no-empty-interface locally
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

No branches or pull requests

2 participants