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

feat: github codespace setup #2598

Closed
wants to merge 6 commits into from
Closed

feat: github codespace setup #2598

wants to merge 6 commits into from

Conversation

ndom91
Copy link
Member

@ndom91 ndom91 commented Aug 24, 2021

Reasoning 💡

This would make it super easy to checkout any PR, issue, etc. in a preconfigured dev env in the browser in the comfortable VS Code interface!

So I added a .devcontainer setup for enabling people to spin up a dev environment with codespaces in no time. Next to where you can set secrets for github actions, you can also set env vars for codespaces. There I set the auth0 id / secret / domain for testing.

What works so far is spinning up the node container, installing the next-auth dependencies, as well as the /app dependencies and starting the dev server. You can then visit the example app homepage via the generated preview URL 🎉

Things that still do not work:

  • copy:css for some reason doesn't ocpy dist/css/index.css to where it should be, although copy:app does work 🤷
  • programmatically getting the codespace preview URL to correctly be able to set NEXTAUTH_URL

Whenever you start a codespace, any ports you forward from the container get a dynamic preview URL like this:
image

You can set env vars in the .devcontainer.json file in a few ways - but I'm not sure how we'd get that dynamic URL ahead of time, or if there is any programmatic way to get it..

You can open up any PR (like this one 😉) in a codespace here:

image

Checklist 🧢

  • Documentation
  • Tests
  • Ready to be merged

Affected issues 🎟

@vercel
Copy link

vercel bot commented Aug 24, 2021

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/CWsBmDp38JbwjMz6H4BVp51BhMa8
✅ Preview: https://next-auth-git-ndom91-dev-container-nextauthjs.vercel.app

@codecov-commenter
Copy link

codecov-commenter commented Aug 24, 2021

Codecov Report

Merging #2598 (6091049) into main (e8a58a0) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff          @@
##            main   #2598   +/-   ##
=====================================
  Coverage   9.90%   9.90%           
=====================================
  Files         84      84           
  Lines       1404    1404           
  Branches     396     396           
=====================================
  Hits         139     139           
  Misses      1039    1039           
  Partials     226     226           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e8a58a0...6091049. Read the comment docs.

@balazsorban44
Copy link
Member

Interesting! After #2552 lands though, the dev app set up might change drastically. I am hoping that vercel/next.js#22867 will simplify things.

Also, could the .devcontainer folder go under config or .github? Would like to reserve the root for only necessary things.

@ndom91
Copy link
Member Author

ndom91 commented Aug 25, 2021

@balazsorban44 unfortunately the .devcontainer directory needs to be in the root as far as I can tell. Regarding the typescript changes, although a lot may change under the hood, in general it'll still be abstracted into a few npm scripts (build, run, etc.), right?

We obviously don't need to go forward with this, but I was experimenting with the devcontainers / codespaces for work and thought it'd be a nice addition here for you and others to easily spin up env's out of PR's, check things out, etc.

@balazsorban44
Copy link
Member

I am not against it, happy to discuss it!

@belgattitude
Copy link
Contributor

belgattitude commented Sep 1, 2021

@balazsorban44

After #2552 lands though, the dev app set up might change drastically.

OMG ! You did this magic 🌟 🥇

I am hoping that vercel/next.js#22867 will simplify things.

Yes indeed and actually even if experimental I feel it's safe to go for it in the next-auth examples. I converted a repo lately without problem.

For NextJS 11.1+, simply enable experimental flag
const nextConfig = {
  experimental: {
    externalDir: true,
  },
};
export default nextConfig;

If there's ome risk to enable experimental, this one works with all next versions since NextJs 10

For NextJS < 11, simply add a webpack override
const nextConfig = {
  webpack: (config, { defaultLoaders }) => {
    // Will allow transpilation of shared packages through tsonfig paths
    // @link https://github.com/vercel/next.js/pull/13542
    const resolvedBaseUrl = path.resolve(config.context, "../../");
    config.module.rules = [
      ...config.module.rules,
      {
        test: /\.(tsx|ts|js|jsx|json)$/,
        include: [resolvedBaseUrl],
        use: defaultLoaders.babel,
        exclude: (excludePath) => {
          return /node_modules/.test(excludePath);
        },
      },
    ];
    return config;
  },
};

@ndom91

unfortunately the .devcontainer directory needs to be in the root as far as I can tell.

I think so too, but maybe not a bad idea... Are you trying to get multiple apps / example running ?

I'm not sure how we'd get that dynamic URL ahead of time, or if there is any programmatic way to get it..

Edit: Just to be sure, you mean to set NEXTAUTH_URL ?

Ah yes this is a little more complex... I'm interested how to find it too. In sandboxes (vercel, netlify), I can use an environment variable for the final url. Don't know about devcontainers though ?

Is it possible to get origin it from request rather than env ? (only in dev)

@balazsorban44
Copy link
Member

balazsorban44 commented Sep 1, 2021

#2631 will fix the dev app.

The Codespace might already have an env variable we could alias in NEXTAUTH_URL?
Looks like this is not needed? @ndom91 when I go to https://some-url-3000.githubpreview.dev/api/auth/session, it seems to work properly.

.devcontainer/devcontainer.json Outdated Show resolved Hide resolved
.devcontainer/devcontainer.json Outdated Show resolved Hide resolved
Co-authored-by: Balázs Orbán <info@balazsorban.com>
@vercel vercel bot temporarily deployed to Preview September 1, 2021 11:34 Inactive
Co-authored-by: Balázs Orbán <info@balazsorban.com>
@vercel vercel bot temporarily deployed to Preview September 1, 2021 11:35 Inactive
@vercel vercel bot temporarily deployed to Preview September 1, 2021 11:35 Inactive
@kriswuollett
Copy link

Interesting! I use devcontainers myself locally based on the "Docker-from-Docker" configuration. If meant to be for development-level activities, I have some suggestions that may be helpful, and some general questions since I'm new to NodeJS development:

  • How does this setup pull in the next-auth code dependencies? In devcontainer.json I just see "context": "../app", so wouldn't ../next-auth/... files not be available in the container, let alone the process.cwd() lines in app/next.config.js working? Is there something I'm missing since this is being tested in Codespaces and not just Visual Studio Code locally with Docker Desktop?
  • Perhaps the Dockerfile should have Next.js start up in debug mode and a launch config for attaching a debugger added? For codespaces specifically, I don't know if the debugging port would need to be exported, but it would be needed for some locally running devcontainer configurations. For an issue I'm having currently, using the debugging has been very useful.
  • Codespaces seems inexpensive, but would we expect project or developers to set up paid account to use it (for primary targeting a codespaces configuration)? I don't see any mention of being free for open-source projects. Are there any of the codespaces limitations this project may hit?
  • Maybe the [...nextauth].js file in the demo app could dynamically add providers only if the required environment variables were set?
  • Given the above with the providers, and not necessarily in an initial PR, maybe it could be updated to be based on Docker-from-Docker compose and bundle something like Keycloak running too? That would allow for the base configuration to have a decently complex setup out of the box without having to customize environment variables in devcontainer.json.

@ndom91
Copy link
Member Author

ndom91 commented Sep 1, 2021

@kriswuollett hey thanks for the all the feedback! So this was just an experimental thing, definitely not 100% thought through yet, at least on my part haha.

But I can hopefully answer some of your questions.

We're not running it in a 'docker-in-docker' type setup, so it clones the repo and runs the container containing node from .devcontainers/Dockerfile and then executes the postCreateCommand string of npm scripts. Our ../app directory was the example application whose package.json linked up one level to the next-auth source code. I'm not 100% sure how it is now that its been refactored to typescript, I haven't had a chance to look into it thoroughly in the past few days. But thats the general gist of it.

Github codespaces currently is free (See blue note box here: https://docs.github.com/en/codespaces/codespaces-reference/understanding-billing-for-codespaces - "Note: Codespaces is free to use for all organizations on a GitHub Team.."). However after reading your message and looking into it, it looks like the initial free period is expiring Sept. 10th, so in just about a week :(

Regarding dynamic providers.. I was talking to Balazs recently about how best we could include some OAuth configs here for testing, without leaking or potentialy exposing our internal testing OAuth keys. Maybe if we conditionally add the configs to [...nextauth].js, something along these lines..

import NextAuth from "next-auth"

const nextAuthConfig = {
  providers: [],
  ..
}

process.env.GITHUB_SECRET && nextAuthConfig.providers.push(GithubProvider({
  ..  
}))

export default NextAuth(nextAuthConfig)

We could allow users to add their own OAuth keys, and it would automatically add / enable that provider if the appropriate provider key / secret is available. Is that kind of what you were picturing? I think thats apretty good way of avoiding having to insert our own keys and potentially losing them, and making it easy for the users / contirbutors to setup and use theirs.

I'm not familiar with Keycloak, but bundling another Auth provider in there to test could be another option, instead of the oauth configs. I'm open to whatever makes it easy for users / contirbutors to get up to speed 👍

@kriswuollett
Copy link

@ndom91, cool. It sounds like all of the next-auth repo is there from the clone. There are so many variations and magic on how they get set up, it can get confusing at times!

Yes, your example with process.env.GITHUB_SECRET && ... is what I was picturing. Not sure if it fits in with this project's / Typescript's style, but the only syntax sugar suggestion I could give is wrapping up each provider in a function and conditionally including via a spread operator in case the conditions get complex. But that could just be my bias from using a lot of Dart recently (collection if). 😀

As far as the devcontainer.json customization goes, I think (code) docs somewhere should list what environment variables to add in containerEnv. I think the format is JSON that supports comments, so all the vars with blank values could exist checked in but commented out. Also be a trick available to help avoid checking in any credentials later by a developer: I think a .gitignore for devcontainer.json could be used and we'd just force add it in case there are any true updates to it?

Let me know if you need any help testing.

For test the app, bundling something like Keycloak in a devcontainer came first to mind, just because I'm currently working with it, and may implement #2637. But perhaps a better option would be something nodejs native like node-oauth2-server since it may not technically require being in its own container. May be worth a separate feature request -- I think it along with this PR could make it easier to debug things like the currently open token refresh and logout issues.

@balazsorban44
Copy link
Member

I don't have strong opinions on this, it looks like there is interest (at least from you @kriswuollett :D ) to develop next-auth in such an environment? Or maybe it would be useful to reproduce bugs (in addition to our CodeSandbox next-auth-example and next-auth-typescript-example).

I don't really see the point of a conditional inclusion of providers. If you don't add the secrets, the given provider simply won't work.

The only thing important to me here is that we are clear that these environments have a public URL, so you should NEVER use an active/prod provider. Other than that, I am probably fine with anything.

@stale
Copy link

stale bot commented Nov 1, 2021

Hi there! It looks like this issue hasn't had any activity for a while. It will be closed if no further activity occurs. If you think your issue is still relevant, feel free to comment on it to keep it open. (Read more at #912) Thanks!

@stale stale bot added the stale Did not receive any activity for 60 days label Nov 1, 2021
@stale
Copy link

stale bot commented Nov 8, 2021

Hi there! It looks like this issue hasn't had any activity for a while. To keep things tidy, I am going to close this issue for now. If you think your issue is still relevant, just leave a comment and I will reopen it. (Read more at #912) Thanks!

@stale stale bot closed this Nov 8, 2021
@balazsorban44 balazsorban44 deleted the ndom91/dev-container branch December 3, 2022 11:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale Did not receive any activity for 60 days
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants