-
-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
refactor: rewrite core to TypeScript #2552
Conversation
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/nextauthjs/next-auth/D5A29RF9vCtjCHNSb3utDMeDbkW5 [Deployment for 76ab39f failed] |
🎉 Experimental release published on npm! npm i next-auth@0.0.0-pr.2552.f73a37a5 yarn add next-auth@0.0.0-pr.2552.f73a37a5 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Half-way reviewing this ✨ , it's really impressive you manage to pull this off in almost one go 🤯 💯 🥇
I leave a few questions/suggestions on the project structure 💭 , I plan to finish the review tomorrow 👍🏽
Conflicts: src/client/react.js src/react/index.tsx types/tests/react.test.ts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to run this locally, but no matter what I did, I could not get it to recognize next-auth/react
.
Kept getting module not found: can't resolve 'next-auth/react'
errors 😢
Balazs and I figured it out!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Super 💯 ⚡️ ✨ , minor last questions/suggestions before merge 💚
*/ | ||
export function merge(target, ...sources) { | ||
/** Deep merge two objects */ | ||
export function merge(target: any, ...sources: any[]) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we can refine the types of this a bit more 💅🏽
export function merge(target: any, ...sources: any[]) { | |
type UnknownObj = Record<string, any> | |
export function merge(target: UnknownObj, ...sources: UnknownObj[]) { |
@@ -17,5 +18,5 @@ export default function Auth0(options) { | |||
} | |||
}, | |||
options, | |||
} | |||
} as any |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to refine here so we don't use any
?
if (userOptions.logger) { | ||
setLogger(userOptions.logger) | ||
} | ||
// If debug enabled, set ENV VAR so that logger logs debug messages | ||
if (userOptions.debug) { | ||
process.env._NEXTAUTH_DEBUG = true | ||
;(process.env._NEXTAUTH_DEBUG as any) = true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can declare the global env variables we rely on like this:
// src/global.d.ts
interface NextAuthEnv {
_NEXTAUTH_DEBUG: 'true' | 'false' | undefined;
// ...
}
interface GlobalProcess {
env: NextAuthEnv;
}
declare const process: GlobalProcess;
function NextAuth(options: NextAuthOptions): any | ||
function NextAuth( | ||
req: NextApiRequest, | ||
res: NextApiResponse, | ||
options: NextAuthOptions | ||
): any |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any chance this will work?
function NextAuthHandler(options: NextAuthOptions): ReturnType<NextAuthHandler>
"lib": ["dom", "dom.iterable", "esnext"], | ||
"allowJs": true, | ||
"skipLibCheck": true, | ||
"strict": false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we can switch to strict: true
and rely on // @ts-expect-error
for the errors we don't want to fix just yet? 💭
All concerns will be addressed in a separate PR, thank you @lluia! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's get this in 💯 🎊 ❤️
* chore(deps): upgrade TS packages * build(ts): use tsc to compile * refactor(ts): move some files to TS * chore: implement SkyPack check suggestions * chore(ci): temprarily disable tests * chore: add PR comment action * chore: add determine version github action * chore: prefix with env. * chore: add runs to action * chore: change runs.using to node12 * chore: fix typo * chore: install @actions/core as dev dependency * chore: move env var, remove old script * chore: change version comment message * refactor(ts): convert server/index.js to TS * chore: fix `types` path * chore: fix paths * refactor(ts): convert `next-auth/react` * refactor(ts): convert `next-auth/jwt` to TS * chore: fix import * refactor: move `types` into `src` * refactor(ts): fix types imports * chore: add cleanup script * chore: exclude all `tests` folder from compilation * refactor: rename types/index.d.ts to types/index.ts * refactor(ts): move `next-auth/jwt` * refactor(ts): move `next-auth/providers` * chore(ts): fix `next-auth` types * refactor(ts): change internal import paths * test(ts): remove type tests * chore: remove test:types script * refactor(ts): move more code to TypeScript * refactor: fix some imports * refactor(ts): move error module into server * fix(ts): add type to .js providers * chore: rename adapters.ts to adapters.d.ts * fix: update exports field * chore: add files that should end up on npm * chore: add stricter lib checking * refactor(ts): remove unnecessary files, fix imports * chore: autocomplete env variables * fix: add css folder to npm files * fix: fix CSS import/generation * feat: log provider when authorization url error happens * refactor(ts): turn pages into .tsx * chore: compile differently for client/server * refactor(ts): move server file to TS * chore: add back node target * chore: add back comment removal * chore: re-enable tests * chore: ignore test files when building * chore(ts): refactor files to TS * chore(ts): fix imports * chore(ts): more ts * fix(ts): correctly type _NEXTAUTH_DEBUG env var * chore: don't generate internals module iwth babel * fix(ts): better `clientId`, `clientSecret` constraints * refactor(ts): move facebook provider to TS * refactor(ts): apply suggested changes * chore(ts): strip internal types from compilation * refactor(ts): move server types to server folder * refactor(ts): rename internals to types
fix dev app imports (check out Experimental feature for allowing importing Typescript files outside of the root directory vercel/next.js#22867)will do in subsequent PR