Skip to content

Conversation

@shrihari-prakash
Copy link

@shrihari-prakash shrihari-prakash commented May 26, 2023

Added types to the package as per #10

@shrihari-prakash shrihari-prakash marked this pull request as ready for review May 26, 2023 18:00
Copy link
Member

@jankapunkt jankapunkt left a comment

Choose a reason for hiding this comment

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

LGTM

@jankapunkt jankapunkt merged commit 19221ac into node-oauth:master May 30, 2023
@cancan101
Copy link

cancan101 commented May 30, 2023

Out of curiosity, why does this PR use typing in package.json and the node-oauth2-server package uses types: https://github.com/node-oauth/node-oauth2-server/blob/aa386b887a8033e8f1a5dfb78c3adde40b7d1784/package.json#L21

I ask because I think there is a typing issue with the node-oauth2-server package now. I have to install @types/oauth2-server to get types to work for that package. I hadn't noticed that before as @types/express-oauth-server transitively imports it.

@HappyZombies
Copy link
Member

@cancan101 There is no difference in these properties, if anything typescript prefers we use "types"

Documentation: https://www.typescriptlang.org/docs/handbook/declaration-files/publishing.html

Screenshot: image

Not sure if this issue is causing your problems? Does using v3.0.0 instead of v3.0.1 cause the same problem? v3.0.1 has the new types definition.

@jankapunkt
Copy link
Member

@cancan101 I didn't know this might cause trouble, I thought they were synonymous (as described in the link). Where do you actually get @types/express-oauth-server? From definitely type? If so, this is the old types, used by the package we forked from and the @types/oauth2-server is from the OAuthJS oauth2-server.

In your case you might simply remove the both @types dependencies and use the builtins, our Oauth2-Server should include index.d.ts in it's published npm packages.

@cancan101
Copy link

If I remove this dependency: @types/oauth2-server (which I know is for the old package), then I get the following TS error:

error TS2307: Cannot find module 'oauth2-server' or its corresponding type declarations. 20 import { AuthorizationCodeModel, AuthorizationCode, User } from "oauth2-server"; 

and I am using the following dependancies (I have tried @node-oauth/express-oauth-server 3.0.0 and 3.0.1):

 "@node-oauth/express-oauth-server": "^3.0.0", "@node-oauth/oauth2-server": "^4.3.0", 
@cancan101
Copy link

cancan101 commented May 30, 2023

Ok, silly me. I fixed that import to be:

import { AuthorizationCodeModel, AuthorizationCode, User, } from "@node-oauth/oauth2-server"; 
@shrihari-prakash
Copy link
Author

@jankapunkt @HappyZombies , in any case, should we change the property to types instead of typings due to TypeScript recommendation?

@HappyZombies
Copy link
Member

@shrihari-prakash yes, I will do that when I wrap up #11

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

Labels

None yet

5 participants