Skip to content

Added type declarations #11

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
May 30, 2023
Merged

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Include types for the package
5 participants