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

Allow the @auth/sveltekit framework to use platform variables for config data #6743

Closed
jschlesser opened this issue Feb 17, 2023 · 2 comments · Fixed by #6744
Closed

Allow the @auth/sveltekit framework to use platform variables for config data #6743

jschlesser opened this issue Feb 17, 2023 · 2 comments · Fixed by #6744
Labels
accepted The feature request is considered. If nobody works actively on it, feel free to. enhancement New feature or request

Comments

@jschlesser
Copy link
Contributor

Description 📓

The current @auth/sveltekit framework assumes that config variables will be available outside of the scope of the hooks handler. Note that in the code below, the SvelteKitAuth function is returning a handler function that is generated anonymously in the framework.

import { GITHUB_ID, GITHUB_SECRET } from "$env/static/private"

export const handle = SvelteKitAuth({
  providers: [GitHub({ clientId: GITHUB_ID, clientSecret: GITHUB_SECRET })],
})

Some platforms, notably cloudflare for my case, provide environment variables via the platform properties which are only available in the hooks handler via the event parameter. Note that the sveltekit hooks Handler has a signature of:

export interface Handle {
	(input: {
		event: RequestEvent;
		resolve(event: RequestEvent, opts?: ResolveOptions): MaybePromise<Response>;
	}): MaybePromise<Response>;
}

Here is a link to the cloudflare sveltekit docs for environment variables as a reference.

Typically to get to platform variables a developer will use event.platform.env.VARIABLE_NAME.
Because of the method of generating the config data outside of the hooks handler, the event object isn't available. I would like to add an optional dynamicOptions parameter that is a function that will generate a config and can have access to the event object. See proposed changes in the linked PR.

How to reproduce ☕️

Unfortunately this is a complicated feature to test because there is a bug with running pnpm run build in the apps/dev/sveltekit directory. It causes vite build to fail to resolve the 'crypto' module. See issue pnpm run build is required to test with the cloudflare local development platform "wrangler". I have 'hacked' the node_modules/@auth/sveltekit module outside of the next_auth development environment and confirmed that the changes do provide access to the platform variables locally on wrangler and also when deployed to cloudflare production. I have tried a couple of ways to get a publishable test working that looks verifiable.

  1. I tried to import from my personal GitHub fork via a git url but workspaces aren't supported that way.
  2. I tried to publish the changes to a local verdaccio npm server but I discovered that the release process is highly customized. I can't simply run pnpm run build and then do regular publish commands. The workspace declarations in package.json aren't getting converted.

Perhaps a core contributor could publish a feature branch so that I could normally import @auth/sveltekit from the feature branch? I could then publish an app to a public GitHub repo that should work just fine. Note that getting the sveltekit demo working on cloudflare is also dependent on #6741 . I opened that issue and I could consolidate these two if necessary for a feature branch to make testing easier.

Contributing 🙌🏽

Yes, I am willing to help implement this feature in a PR

@jschlesser jschlesser added enhancement New feature or request triage Unseen or unconfirmed by a maintainer yet. Provide extra information in the meantime. labels Feb 17, 2023
@jschlesser
Copy link
Contributor Author

After playing around with this, I realize that I need to do two things.

  1. The dynamicOptions function needs to possibly be async
  2. I think the function that the framework needs a return type that is possibly a promise because of Provider a simpler interface #1

I need some time to work on this to make sure the types are all correct. Ill move the PR to a draft.

@ThangHuuVu ThangHuuVu added accepted The feature request is considered. If nobody works actively on it, feel free to. and removed triage Unseen or unconfirmed by a maintainer yet. Provide extra information in the meantime. labels Feb 25, 2023
@ThangHuuVu
Copy link
Member

This makes good sense. Let's work on the PR together 💪 Thanks for filing the issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted The feature request is considered. If nobody works actively on it, feel free to. enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants