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

Typescript support #220

Closed
wants to merge 9 commits into from
Closed

Typescript support #220

wants to merge 9 commits into from

Conversation

ubbe-xyz
Copy link
Collaborator

@ubbe-xyz ubbe-xyz commented Jun 6, 2020

Summary

Following the effort @falleco started on #71, this PR aims to bring updated types for v2.

🗺  Roadmap

🚨   Disclaimer

I've been working with TS for a while, but this is my first time typing a library so feel free to comment extensively on what can be improved or might be incorrect. 🙏🏻

I'm also not familiar with typeorm or the inner workings of both adapters and provider so @iaincollins your guiding light 💡 here much appreciated 🤞🏻

@vercel
Copy link

vercel bot commented Jun 6, 2020

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/iaincollins/next-auth-docs/ah4xm0hva
✅ Preview: https://next-auth-docs-git-fork-lluia-chore-types.iaincollins.vercel.app

@vercel vercel bot temporarily deployed to Preview June 6, 2020 17:30 Inactive
@iaincollins
Copy link
Member

Thanks @lluia, I really appreciated you picking up the work from @falleco for v1! I left the old PR both as reference for myself, and hoping someone might be able to help with it.

I just wanted to say it will probably be a few days before I can pick this up again, but it's really appreciated and it is something I would like to have.

One thing it would be really nice to have some developer tests or a script that we could run with an npm run command to help indicate what is missing types so we can improve that over time.

We already have linting with eslint, using StandardJS (npm run lint), if using a linter plugin is helpful there? Any recommendations on how we could do something like that would be really welcome. My reason for asking is that I would want to be sure we maintain them easily and they don't break over time when we change something and forget to update types.

@shamoons
Copy link

shamoons commented Jun 8, 2020

Looking forward to this

@vercel vercel bot temporarily deployed to Preview June 8, 2020 16:43 Inactive
@ubbe-xyz
Copy link
Collaborator Author

ubbe-xyz commented Jun 8, 2020

@iaincollins after some investigation I decided to go with dtslint for linting the declaration files.

It seems is what most libraries are using 🧐 ( given is my first try doing this happy to move to other suggestions... )

Currently, it can't compile due to an error on next types ( which we depend on )

$ npx dtslint types
Error: At /Users/lluis.agusti/Code/next-auth/node_modules/next/types/index.d.ts:{"line":14,"character":6}: 'ts-ignore' is forbidden.
    at /Users/lluis.agusti/Code/next-auth/node_modules/dtslint/bin/index.js:198:19
    at Generator.next (<anonymous>)
    at fulfilled (/Users/lluis.agusti/Code/next-auth/node_modules/dtslint/bin/index.js:6:58) 

I'm looking on how to bypass that 🤔 🤞🏻

@vercel vercel bot temporarily deployed to Preview June 8, 2020 16:57 Inactive
@iaincollins
Copy link
Member

@iaincollins after some investigation I decided to go with dtslint for linting the declaration files.

It seems is what most libraries are using 🧐 ( given is my first try doing this happy to move to other suggestions... )

Currently, it can't compile due to an error on next types ( which we depend on )

$ npx dtslint types
Error: At /Users/lluis.agusti/Code/next-auth/node_modules/next/types/index.d.ts:{"line":14,"character":6}: 'ts-ignore' is forbidden.
    at /Users/lluis.agusti/Code/next-auth/node_modules/dtslint/bin/index.js:198:19
    at Generator.next (<anonymous>)
    at fulfilled (/Users/lluis.agusti/Code/next-auth/node_modules/dtslint/bin/index.js:6:58) 

I'm looking on how to bypass that 🤔 🤞🏻

Thanks! This seems like a good direction!

This is not an area I'm familiar with myself, but if we can get one or two examples working I'd be very happy to pitch in and try and improve (and maintain) coverage.

@ubbe-xyz
Copy link
Collaborator Author

@iaincollins I opened an issue on dtslint to see if I can get help with the error above 💭
microsoft/dtslint#297

I was thinking instead from writing the declaration files manually 🤔 , we could move the library to TS and let the compiler generate the types for us ( might be more scalable ). It'll involve though some work though 😆

@iaincollins iaincollins changed the base branch from master to main June 13, 2020 18:56
@arnodel
Copy link

arnodel commented Jun 15, 2020

I tried to start adding types to for the client/ directory but could only make it work by adding a client.d.ts file to the root of the repo (next to client.js). How can I put these definitions within the types directory instead?

@vercel vercel bot temporarily deployed to Preview June 15, 2020 17:11 Inactive
@ubbe-xyz
Copy link
Collaborator Author

ubbe-xyz commented Jun 15, 2020

@arnodel I think the trick is to declare the module in this way:

declare module 'next-auth/client' {
  // type declarations here
}

in this way, it won't matter where declaration files are located ( as per TS docs ) and hence we can make it live inside of types/ ( keeping the project root cleaner in this way ) 💅🏻

I added an example of it working 👍🏻

@vercel vercel bot temporarily deployed to Preview June 15, 2020 17:46 Inactive
@iaincollins
Copy link
Member

^ Thank you to everyone working on this, I'm not commenting because I don't have anything to add to the convo but this is very much appreciated. 🙂

@LoriKarikari
Copy link
Contributor

Not into TypeScript @iaincollins?

@iaincollins
Copy link
Member

@LoriKarikari I prefer strongly typed languages in general - C# is my all time favourite language - but I'm not personally a fan of TypeScript in practice, as I've run into too many issues with it (e.g. edge cases, compatibility issues, etc).

If pushed I would say I prefer Flow, however, I get that TypeScript is the most popular way of using types in JavaScript and a lot of folks get value out of it and I'm happy to support it.

I'm looking forward to seeing Deno evolve though! I think having better out of the box for types (fast and native) will make it a very different developer experience.

@arnodel
Copy link

arnodel commented Jun 15, 2020

Thanks @lluia for the example. However it doesn't appear to work for me with the new change - I cannot import from next-auth or next-auth/client. I have attached screenshots from my IDE (vscode). I must admit that although I know typescript well enough, the type declarations for javascript node modules are mysterious to me - so it is possible that I am under some basic misapprehension.

Screenshot 2020-06-15 at 19 57 49

Screenshot 2020-06-15 at 19 57 59

Previously those imports were resolved (with types/index.d.ts not containing a module declaration and client.d.ts at the module root).

@ubbe-xyz
Copy link
Collaborator Author

@arnodel where are you trying to import those types? Currently they're not published hence not consumable on a project.

We'reusing TDSLint for now to help us write the declaration files which you can do through yarn lint:types. 🤔

Sent with GitHawk

@arnodel
Copy link

arnodel commented Jun 16, 2020

@lluia I am importing from a project that uses next-auth :). So let me check that I understand: currently the type declaration files are not published, later they will be put in dist/, alongside the js files that they provide type declarations for? I would find it useful to have that in place as I am currently using it. Currently I am using the a workaround which consists of addings the following compilerOptions to the tsconfig.json of the project that uses next-auth:

   "baseUrl": ".",
    "paths": {
      "next-auth/*": ["node_modules/next-auth/types/*"]
    }

That doesn't seem a good long term solution to me however (no other package that I depend on requires that).

Also, when running yarn lint:types I got an error:

Error: /Users/adelobelle/personal/next-auth/types/client.d.ts:2:1
ERROR: 2:1  no-single-declare-module  File has only 1 module declaration — write it as an external module. See: https://github.com/Microsoft/dtslint/blob/master/docs/no-single-declare-module.md

The link says that declare module should "typically be avoided" - and to me it makes senses if the files are going to end up in dist/ as I describe above.

So perhaps it would be better to rewrite the contents of index.d.ts and client.d.ts without the declare module statements? I have made it work by changing the compilerOptions/paths configutation of types/tsconfig.json slightly - see arnodel@3260060

@ubbe-xyz
Copy link
Collaborator Author

@arnodel good catch, I think as you suggest using compilerOptions/paths for now seems a better approach than declare module 💯 .

To clarify: for now, the types/* directory helps us keep the types in one place and lint them through dtslint. Later, once we're happy with the types, we will need to copy what's in types/*.d.ts to dist/ at build time, so we have them published on NPM and clients of this library can consume them.

If you need them straight away for a project you're working I'd suggest the best is to use npm link with this branch 🤔

@arnodel
Copy link

arnodel commented Jun 16, 2020

@lluia personally I wouldn't mind having the type definition files in the src/ directory. I feel like having them close to the js files makes the link more obvious, but I don't actually know what is generally done. The dependencies I use seem to either be written in Typescript or have a separate @types package.

I also think it would be good to publish those types as soon as possible. IMHO as this is a fork / PR, there is no need to worry about exposing a non-stable interface to unsuspecting users, but it allows people to use it easily (knowing the risks).

In my opinion lack of typescript definitions is likely to put off a fair number of potential users, so providing something usable early would be very valuable.

@vercel vercel bot temporarily deployed to Preview July 18, 2020 18:57 Inactive
@samsamson33
Copy link

@lluia what's happening with the typings now? I see that the CI is failing on DefinitelyTyped... is there anything I can do help out?

@ubbe-xyz
Copy link
Collaborator Author

@samsamson33 I need to have a look, hopefully, this week 👀 🤞🏻 , I think the compiler is trying to parse the types from next and failing:

##[error]node_modules/next/dist/next-server/lib/router/router.d.ts(72,21): error TS2304: Cannot find name 'PopStateEvent'.

A workaround, for the time being, is to not relly on next types and type those manually. I'll give it a go 👨🏻‍💻 ( feel free to suggest any improvements or changes you see fit 💯 )

@samsamson33
Copy link

@lluia could not finding PopStateEvent possibly be because you don't have "dom" in your tsconfig's lib? (Sorry if I'm way off the mark here)

@ubbe-xyz
Copy link
Collaborator Author

@samsamson33 ah good point 💥 , let me push it to the PR and see if it makes the error go away 🤞🏻

@NickBolles
Copy link
Contributor

NickBolles commented Jul 30, 2020

@iaincollins @lluia @samsamson33 @bamorim and others. awesome work on this! (I was disappointed that my original work on typings weren't updated for this release, so I came looking)

I spent an hour or two trying out converting next auth to typescript. I just have to finish typing the options and a few other small issues (and taking some types from this PR). @iaincollins would you be more open to accepting a PR like this? Most of it is utilizing type inference or any. but at the very least it's up to date and can be improved incrementally with (hopefully) no impact on existing users (js or ts), I've added minimal interfaces where it makes sense, and fixed a few type errors where necessary. I'd be curious to hear your objections and drawbacks of typescript (beyond initial development, because I think everyone agrees with that point)

@NickBolles NickBolles mentioned this pull request Jul 30, 2020
7 tasks
@NickBolles
Copy link
Contributor

@iaincollins @lluia @samsamson33 @bamorim See the PR I just submitted for a working migration to typescript. I'm sure there's more work to be done, so please check it out and provide feedback!

We might end up abandoning it, because of reasons explained earlier in this chain, but I thought I'd remove the "too much work to transition" hurdle. (because really its not hard... we could have stopped at just a rename and add a bunch of anys, but I wanted to do it right).

@iaincollins
Copy link
Member

I favour @lluia approach in this PR, which is incremental and gives a chance to test things out as a team.

With regard to TypeScript, supporting pull request is my priority right now.

If anyone is looking for a way to help out to support a move to TypeScript (beyond helping out with this PR) the most useful thing right now would be to help out with the end to end tests, which have sadly been on hold for too long.

@iaincollins iaincollins added enhancement New feature or request help needed The maintainer needs help due to time constraint/missing knowledge labels Jul 30, 2020
@ubbe-xyz
Copy link
Collaborator Author

ubbe-xyz commented Aug 3, 2020

Update 🗓

So the types are finally ready on DefinitivelyTyped 💆🏻‍♂️ 🎊 🤞🏻 💥
DefinitelyTyped/DefinitelyTyped#45775

I'll need now a couple of reviews (maybe @iaincollins and @NickBolles ?) to be able to merge it 💚

I'm aware they're less than a perfect but I think is a good start and we can get them out and iterate later.

I think the key for them to grow strong is to sync well whenever changes happen in this library. As a first step making CHANGELOG.md would be good, but here's more 🥗 for thought.

Why it took so long ⏲

I was caught forever with a cryptic error related to 'source-map' that went away once I removed next types from the dependencies. This, however, means I'm typing NextApiRequest and NextApiResponse manually which seems a bit brittle and I'm eager to see how it plays once the types are released...

The fact also that I lost my job due to COVID and been actively looking for a new one since a few weeks made a bit challenging keeping up with open-source 😅

types/index.d.ts Show resolved Hide resolved
types/index.d.ts Show resolved Hide resolved
types/providers/index.d.ts Show resolved Hide resolved
@ubbe-xyz
Copy link
Collaborator Author

ubbe-xyz commented Aug 9, 2020

@m5r thanks for your comments 💯 , I'll try to get them addressed soon. Could you next time comment on the DefinitivelyTyped PR? The work is carried on there and this one is just open for reference.

@ubbe-xyz
Copy link
Collaborator Author

Finally @types/next-auth is published, we can continue to raise PRs in DefinitivelyTyped to improve them 💚 💯

@ubbe-xyz ubbe-xyz closed this Aug 29, 2020
@LoriKarikari
Copy link
Contributor

Great work @lluia!

@iaincollins
Copy link
Member

@lluia Thank you and congratulations! <3

What are your thoughts on merging this PR into NextAuth.js also, so we can work towards better support for types natively?

@alxyuu
Copy link

alxyuu commented Aug 31, 2020

are there any plans to add types for next-auth/jwt as well?

@ubbe-xyz
Copy link
Collaborator Author

ubbe-xyz commented Sep 1, 2020

@iaincollins I think the types on this PR are a bit outdated, so in that case would be best to directly copy what we have on DefinitivelyTyped 🤔

I'd say for now lets see how @types/next-auth plays out, there's people making PRs on it 🎊 , too bad I'm on holidays this week 🏖️ with limited access to internet, I wanted to find time to create the example TS repo 🤞🏽

@JuanM04
Copy link

JuanM04 commented Sep 1, 2020

I could do an exact copy of the example repo but in TypeScript. Just create an empty repo in the organization and I'll PR it

@JuanM04
Copy link

JuanM04 commented Sep 2, 2020

are there any plans to add types for next-auth/jwt as well?

@alxyuu, I just did them, here is the pull request!

@ubbe-xyz
Copy link
Collaborator Author

ubbe-xyz commented Sep 5, 2020

@JuanM04 here you go if you have time 👍🏽 , would be cool to keep the file structure same as the default example repo.

In case you don't find time don't worry! I'll try to PR on it once I'm back from hols ⛱ next week ( @iaincollins )

@zenflow
Copy link
Contributor

zenflow commented Sep 5, 2020

@lluia Thanks for this contribution, and more importantly, enjoy your holidays! 😃

@JuanM04
Copy link

JuanM04 commented Sep 6, 2020

Thank you, @lluia! Enjoy your holidays!

@JuanM04
Copy link

JuanM04 commented Sep 6, 2020

I finished the repo, but some types are wrong. I opened a PR fixing them

@iaincollins
Copy link
Member

@lluia Thanks for your feedback! Sounds good to me. Have a great holiday! :-)

@kwent
Copy link

kwent commented Sep 10, 2020

Thank you. Should be added in the documentation as well :) with the command npm install @types/next-auth --save-dev

@mobula9
Copy link

mobula9 commented Sep 24, 2020

great !! thx

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help needed The maintainer needs help due to time constraint/missing knowledge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet