Nekonyx / next-auth-steam

Steam authentication provider for next-auth
70 stars 18 forks source link

getServerSession example using app router #4

Closed bryanzack closed 1 year ago

bryanzack commented 1 year ago

Hey, thanks for your reply to my question about the use of getServerSession in your library. I have taken the information you gave me and seem to have created a working, build-able example which uses getServerSession alongside the new app router. Even though it builds, it does appear to be a bit janky. The main differences between my fork and your app router example are my route.ts and me/page.tsx files.

Two main sources of jankiness are here (where I define the authOptions provider details & callbacks you provided) and here (where I call getServerSession with the authOptions as an argument).

I am not that well-versed in Typescript so I rely on @ts-expect-error to make it work (maybe @qwertyuiop6 might know the proper implementation?), but I am almost confident that I am passing provider details into getServerSession improperly.

Let me know what you think!

Nekonyx commented 1 year ago

Hey, thanks for contributing.

Sorry to bother you, @qwertyuiop6. Could you take a look at this PR and tell me if all is well here? I still have no experience with app routing.

qwertyuiop6 commented 1 year ago

@bryanzack I'm sorry, I just noticed this message. I took a look at the implementation of next-auth . The authOptions of getServerSession does not require providers.

So you can simply change getAuthOptions to be like this:

export function getAuthOptions(req?: NextRequest): AuthOptions {
    return {
        providers: req
            ? [
                    SteamProvider(req, {
                        clientSecret: process.env.STEAM_SECRET!,
                        callbackUrl: 'http://localhost:3000/api/auth/callback',
                    }),
              ]
            : [],
        callbacks: {
            jwt({ token, account, profile }) {
                if (account?.provider === PROVIDER_ID) {
                    token.steam = profile;
                }
                return token;
            },
            session({ session, token }) {
                if ('steam' in token) {
                    // @ts-expect-error
                    session.user.steam = token.steam;
                }
                return session;
            },
        },
    };
}
bryanzack commented 1 year ago

Thanks for the clarification @qwertyuiop6! Seems to make sense to me now too. @Nekonyx time permitting, let me know if there is anything I can do to make it merge ready.

bryanzack commented 1 year ago

Always appreciate another another set of eyes, haven't ever done a PR before this one. :)

Nekonyx commented 1 year ago

@bryanzack please don't change package.json/pnpm-lock.yaml related to module itself. It scares me a bit x)

bryanzack commented 1 year ago

@Nekonyx Ah okay, sorry about that. Just so I know, should PR's not include those files?

Nekonyx commented 1 year ago

@Nekonyx Ah okay, sorry about that. Just so I know, should PR's not include those files?

If the module works well with next@^13.3.4 and does not require next@^13.4.4, it is better not to raise the minimum supported version of Next

bryanzack commented 1 year ago

@Nekonyx Ah okay, sorry about that. Just so I know, should PR's not include those files?

If the module works well with next@^13.3.4 and does not require next@^13.4.4, it is better not to raise the minimum supported version of Next

Good point! I hadn't even thought about that

qwertyuiop6 commented 1 year ago

@Nekonyx Ah okay, sorry about that. Just so I know, should PR's not include those files?

If the module works well with next@^13.3.4 and does not require next@^13.4.4, it is better not to raise the minimum supported version of Next

Hi @Nekonyx, nextjs launched a stable version of the app router in version 13.4. previously, this was an experimental feature. in 13.4 additional fields need to be declared in next.config.js are no longer needed, I think we should support the latest stable version. Instead of staying in the experimental version.

Maybe we should do a feasibility test and upgrade the nextjs dependency in all the examples to the latest.

Nekonyx commented 1 year ago

It doesn't matter if next.js only upgraded the minor version. The module will continue to support both next.js 13.3, 13.4, and any 13.x versions until the major version is changed to 14.

So I'll merge it to another branch, to revert changes in package.json/pnpm-lock.yaml to not bother you guys. Thank you very much for your work, guys, and sorry for it's taking too long. <3