fix(ts): improve `events` handlers' types #1853

zackdotcomputer · 2021-04-25T16:59:36Z

What:

This change introduces typing for the events object so that developers using Typescript with next-auth will receiving hinting as to the type of the incoming arguments to events callback functions.

Resolves #1639

Why:

Because previously developers didn't have any idea what arguments they'd receive when listening to events even though it was relatively well-defined in the source code.

How:

By analyzing the source code for the possible paths to trigger events and then editing the typing files to match the discovered pathways.

Checklist:

Documentation (I think N/A since this basically is documentation?)
Tests
Ready to be merged

vercel · 2021-04-25T16:59:43Z

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/nextauthjs/next-auth/Bro6i1RYYHGXpXhNwjeir57EwAt5
✅ Preview: https://next-auth-git-fork-zackdotcomputer-main-nextauthjs.vercel.app

balazsorban44

Thank you!

Could you please also update the docs in these places:

https://github.com/nextauthjs/next-auth/blob/main/www/docs/configuration/options.md#description-7

https://github.com/nextauthjs/next-auth/blob/main/www/docs/configuration/events.md

ubbe-xyz

Thanks 💯 , brilliant work; I leave a couple of suggestions 👍🏽

types/index.d.ts

balazsorban44

Could you please fix the merge conflicts?

types/index.d.ts

balazsorban44 · 2021-04-28T22:59:58Z

Also my previous comment still stands: #1853 (review)

zackdotcomputer · 2021-04-28T23:02:07Z

Sure thing @balazsorban44 - I was just starting on the code comments before tackling the documentation so I was sure to be documenting my final draft. Will tackle the merge and your code comments and then wrap up the docs changes.

zackdotcomputer · 2021-04-29T00:20:54Z

All comments responded to and documentation updated.

ubbe-xyz

Just one small thing to prevent conflicts on the documentation 🙏🏽 , thanks a lot for this massive work 💯

ubbe-xyz · 2021-05-01T09:13:17Z

www/docs/configuration/options.md

@@ -5,7 +5,7 @@ title: Options

 ## Environment Variables

-### NEXTAUTH_URL 
+### NEXTAUTH_URL


Could we revert this file?

I have a PR open updating it extensively and I think it's gonna end a bit in conflict hell 😬 due to the prettier changes. I'll add the bit you updated pointing to the events documentation on my PR 👍🏽

In fact, NVM I confused the file name 🤦🏽‍♂️ ☕️ , ha ha we're all good 💚

ubbe-xyz

💪🏽

ubbe-xyz · 2021-05-01T09:15:26Z

www/docs/configuration/options.md

@@ -5,7 +5,7 @@ title: Options

 ## Environment Variables

-### NEXTAUTH_URL 
+### NEXTAUTH_URL


In fact, NVM I confused the file name 🤦🏽‍♂️ ☕️ , ha ha we're all good 💚

types/index.d.ts

balazsorban44

Some feedback from me! Please remove the extraneous generics, as we discussed Module Augmentation. Otherwise, it is looking good and will be a nice improvement, so thank you! 🙏

I am also thinking that most of these should be moved to internals/events.d.ts maybe to keep down the index.d.ts file size as well. What do you think @lluia?

types/index.d.ts

zackdotcomputer · 2021-05-05T14:50:02Z

Resolved merge conflict, updated to main repo's main branch, applied all suggested changes except for keeping the createUser, updateUser, and linkAccount in the common events interface since I think those are sent even if you are using JWTs.

balazsorban44 · 2021-05-05T15:09:20Z

Thabk you!

About the events, they aren't, you can see it in the source code:

https://github.com/nextauthjs/next-auth/blob/17b789822de64eb845e1e8e49ea83dbff56344f4/src/server/lib/callback-handler.js

See the following lines: 95, 100, 154, 199, 210

those events will only be triggered, if an adapter is used. there is no point in triggering them otherwise.

EDIT: Sorry for my quick judgment on this, I'll have a proper look later on from the desktop. Feel free to leave your changes for now, you might be right, I'll get back on this.

Thank you for your patience on this! 🙏

zackdotcomputer · 2021-05-05T15:26:11Z

Ah no worries, it sounds like we're actually reading this the same way @balazsorban44 that those events are sent based on whether or not you have an adapter connected, which as far as I can tell is separate from whether you are using JWTs (since you could configure an adapter and also set session: { jwt: true } I think). If we want to get precise then, I think we wind up needing interface HasAdapterEventCallbacks with these three methods and then the final type is type EventCallbacks = JWTEventCallbacks | ((JWTEventCallbacks | SessionEventCallbacks) & HasAdapterEventCallbacks)

But are we now splitting hairs? Let me know if you think that's excessive or not - if not I can restructure the type like that pretty easily.

balazsorban44 · 2021-05-05T15:33:00Z

Right! I think it's a big improvement anyway, so I'll approve this in its current form, and if for some reason it won't be good enough, we can open a new PR. Thank you for your contribution! 💚

balazsorban44 · 2021-05-05T17:33:48Z

@zackdotcomputer sorry, this is the last thing I'll ask, but before merging, could you please fix the failing tests?

balazsorban44

There are some failing tests

zackdotcomputer · 2021-05-05T21:19:43Z

Okay - we'll see how the build goes but that should have fixed it.

balazsorban44 · 2021-05-06T07:20:12Z

@zackdotcomputer Unfortunately it still fails. I recommend running npm run test locally before you commit and push.

zackdotcomputer · 2021-05-06T09:16:45Z

Ah yup my bad @balazsorban44 - I was running the tests but not the lint. Fixed the lint now too - run test is green locally.

balazsorban44 · 2021-05-06T09:42:15Z

Yeah, we don't really have actual tests at the moment. 🥲. Working on it though! #1937

zackdotcomputer requested review from balazsorban44 and ubbe-xyz as code owners April 25, 2021 16:59

vercel bot temporarily deployed to Preview April 25, 2021 16:59 Inactive

github-actions bot added test Related to testing TypeScript Issues relating to TypeScript labels Apr 25, 2021

balazsorban44 requested changes Apr 25, 2021

View reviewed changes

ubbe-xyz requested changes Apr 25, 2021

View reviewed changes

types/index.d.ts Outdated Show resolved Hide resolved

types/index.d.ts Outdated Show resolved Hide resolved

types/index.d.ts Outdated Show resolved Hide resolved

balazsorban44 requested changes Apr 28, 2021

View reviewed changes

types/index.d.ts Outdated Show resolved Hide resolved

types/index.d.ts Outdated Show resolved Hide resolved

zackdotcomputer added 6 commits April 29, 2021 00:06

Constrain the adapters type generics more accurately

01d9a81

Add types for the incoming messages to events callbacks

f5ca3c1

Code review comments from @lluia

6ef0be8

Rebase from trunk and fix merge conflicts

c9d3fa1

Update documentation

3178f08

Rip out generics

e0c5c86

vercel bot temporarily deployed to Preview April 29, 2021 00:19 Inactive

github-actions bot added the documentation Relates to documentation label Apr 29, 2021

zackdotcomputer requested review from balazsorban44 and ubbe-xyz April 29, 2021 00:20

ubbe-xyz requested changes May 1, 2021

View reviewed changes

ubbe-xyz approved these changes May 1, 2021

View reviewed changes

Merge branch 'main' into main

8411a23

vercel bot temporarily deployed to Preview May 1, 2021 09:16 Inactive

ubbe-xyz reviewed May 1, 2021

View reviewed changes

types/index.d.ts Outdated Show resolved Hide resolved

balazsorban44 requested changes May 1, 2021

View reviewed changes

fix(build): export aliases from client (#1909)

9ab4d38

github-actions bot added core Refers to `@auth/core` providers labels May 5, 2021

zackdotcomputer requested a review from balazsorban44 May 5, 2021 14:50

balazsorban44 approved these changes May 5, 2021

View reviewed changes

balazsorban44 changed the title ~~Add typing for events handlers' message arguments (Resolves #1639)~~ fix(ts): improve events handlers' types May 5, 2021

Merge branch 'main' into main

5de8696

vercel bot temporarily deployed to Preview May 5, 2021 15:40 Inactive

github-actions bot removed client Client related code core Refers to `@auth/core` providers labels May 5, 2021

balazsorban44 self-requested a review May 5, 2021 17:33

balazsorban44 requested changes May 5, 2021

View reviewed changes

Fix tests

c9da56c

vercel bot temporarily deployed to Preview May 5, 2021 21:19 Inactive

zackdotcomputer requested a review from balazsorban44 May 5, 2021 21:19

Fix lint error

023d447

vercel bot temporarily deployed to Preview May 6, 2021 09:16 Inactive

Merge branch 'main' into main

8fccd88

vercel bot temporarily deployed to Preview May 6, 2021 09:16 Inactive

balazsorban44 approved these changes May 6, 2021

View reviewed changes

balazsorban44 merged commit ffd0601 into nextauthjs:main May 6, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(ts): improve `events` handlers' types #1853

fix(ts): improve `events` handlers' types #1853

zackdotcomputer commented Apr 25, 2021 •

edited

vercel bot commented Apr 25, 2021 •

edited

balazsorban44 left a comment

ubbe-xyz left a comment

balazsorban44 left a comment

balazsorban44 commented Apr 28, 2021

zackdotcomputer commented Apr 28, 2021

zackdotcomputer commented Apr 29, 2021

ubbe-xyz left a comment

ubbe-xyz May 1, 2021

ubbe-xyz May 1, 2021

ubbe-xyz left a comment

ubbe-xyz May 1, 2021

balazsorban44 left a comment •

edited

zackdotcomputer commented May 5, 2021

balazsorban44 commented May 5, 2021 •

edited

zackdotcomputer commented May 5, 2021

balazsorban44 commented May 5, 2021 •

edited

balazsorban44 commented May 5, 2021 •

edited

balazsorban44 left a comment

zackdotcomputer commented May 5, 2021

balazsorban44 commented May 6, 2021

zackdotcomputer commented May 6, 2021

balazsorban44 commented May 6, 2021 •

edited

fix(ts): improve events handlers' types #1853

fix(ts): improve events handlers' types #1853

Conversation

zackdotcomputer commented Apr 25, 2021 • edited

vercel bot commented Apr 25, 2021 • edited

balazsorban44 left a comment

Choose a reason for hiding this comment

ubbe-xyz left a comment

Choose a reason for hiding this comment

balazsorban44 left a comment

Choose a reason for hiding this comment

balazsorban44 commented Apr 28, 2021

zackdotcomputer commented Apr 28, 2021

zackdotcomputer commented Apr 29, 2021

ubbe-xyz left a comment

Choose a reason for hiding this comment

ubbe-xyz May 1, 2021

Choose a reason for hiding this comment

ubbe-xyz May 1, 2021

Choose a reason for hiding this comment

ubbe-xyz left a comment

Choose a reason for hiding this comment

ubbe-xyz May 1, 2021

Choose a reason for hiding this comment

balazsorban44 left a comment • edited

Choose a reason for hiding this comment

zackdotcomputer commented May 5, 2021

balazsorban44 commented May 5, 2021 • edited

zackdotcomputer commented May 5, 2021

balazsorban44 commented May 5, 2021 • edited

balazsorban44 commented May 5, 2021 • edited

balazsorban44 left a comment

Choose a reason for hiding this comment

zackdotcomputer commented May 5, 2021

balazsorban44 commented May 6, 2021

zackdotcomputer commented May 6, 2021

balazsorban44 commented May 6, 2021 • edited

fix(ts): improve `events` handlers' types #1853

fix(ts): improve `events` handlers' types #1853

zackdotcomputer commented Apr 25, 2021 •

edited

vercel bot commented Apr 25, 2021 •

edited

balazsorban44 left a comment •

edited

balazsorban44 commented May 5, 2021 •

edited

balazsorban44 commented May 5, 2021 •

edited

balazsorban44 commented May 5, 2021 •

edited

balazsorban44 commented May 6, 2021 •

edited