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

[Feature Request]: Migrate to ES module syntax #1074

Closed
juliusmarminge opened this issue Sep 5, 2023 · 8 comments
Closed

[Feature Request]: Migrate to ES module syntax #1074

juliusmarminge opened this issue Sep 5, 2023 · 8 comments
Labels
feature request New feature requests v3 Version 3 target

Comments

@juliusmarminge
Copy link

Description

Would be better if Lucia supported module augmentation through ES module syntax:

declare module "lucia" {
  type Auth = OurAuth;
  interface DatabaseUserAttributes {
    github_username: string;
  }
}

This would improve monorepo support as we could put this declaration in a .ts file (.d.ts files kinda suck in monorepos) and better align with what other repos are doing:

  • NextAuth:
declare module "next-auth" {
  interface Session {
    user: {
      id: string;
    } & DefaultSession["user"];
  }
}
  • Tanstack Router:
export function createRouter() {
  const router = new Router( ... )

  return router;
}

declare module '@tanstack/router' {
  interface Register {
    router: ReturnType<typeof createRouter>
  }
}

As of now, I can't seem to get this to properly work in a monorepo as declaring types in .d.ts files don't propagate well through multiple packages

@juliusmarminge juliusmarminge added the feature request New feature requests label Sep 5, 2023
@hdwatts
Copy link
Contributor

hdwatts commented Sep 10, 2023

+1 - It would also be following Microsoft's recommendations

It is also worth noting that, for Node.js applications, modules are the default and we recommended modules over namespaces in modern code.

To note, @juliusmarminge I was able to get this working in a create-t3-turbo repo. I'm continuing to play around, but let me know if there's anything I'm potentially missing. The need for an additional .d.ts file isn't ideal, but I haven't been able to avoid it.

  1. Create app.d.ts in packages/auth and follow the lucia recommendation
// File: /packages/auth/app.d.ts

/// <reference types="lucia" />
declare namespace Lucia {
  export type Auth = import("./src/lucia").Auth;
  // export type DatabaseUserAttributes =
  // import("./types").DatabaseUserAttributes;
  export interface DatabaseUserAttributes {
    username: string;
  }
  export interface DatabaseSessionAttributes {}
}
  1. Confirm it works in the local auth package by creating an invalid database user attributes call
    File: /packages/auth/src/lucia.ts
image
  1. In my nextjs app create an app.d.ts and reference one in @acme/auth
// File: /apps/nextjs/app.d.ts

/// <reference types="@acme/auth/app.d.ts" />
  1. Create an invalid createUser call in my nextjs app
    File: /apps/nextjs/src/app/api/auth/signup/route.ts
image

@juliusmarminge
Copy link
Author

Step 3 is a workaround around the problem though. Using modules it just works

@hdwatts
Copy link
Contributor

hdwatts commented Sep 10, 2023

Step 3 is a workaround around the problem though. Using modules it just works

Yup, definitely agreed modules are the way to go, hence my +1. Was just offering a workaround to help with your comment:

As of now, I can't seem to get this to properly work in a monorepo as declaring types in .d.ts files don't propagate well through multiple packages

@pilcrowOnPaper
Copy link
Member

Hi, sorry for the late response. I tried it out and it looks to work fine. My only issue is that I'm not sure if I can support both namespaces and modules, in which case the change has to be part of v3.

@tobiasdiez
Copy link
Contributor

Another idea would be to completely drop the augmentation and simply specify the types as generics to the adapter (if needed, e.g. prisma should be able to extract the type from the model).

tobiasdiez added a commit to JabRef/JabRefOnline that referenced this issue Sep 24, 2023
- Once redis/ioredis#1822 is fixed, remove
patch to ioredis
- Once Azure/azure-functions-host#162 is
fixed, remove patch to nitro that wraps console log.
- Once unjs/nitro#1753 is merged and released,
remove corresponding patch to nitro
- Once lucia-auth/lucia#1153 is fixed, rename
models/fields in prisma
- Once lucia-auth/lucia#1155 is merged and
released, remove custom h3 lucia middleware
- Once lucia-auth/lucia#1074 is fixed, remove
lucia types shims
- Enable CSRF protection in lucia (but then login doesn't work
anymore...)
@pilcrowOnPaper
Copy link
Member

pilcrowOnPaper commented Sep 26, 2023

I've been playing around with this for a while, and yeah, I can't support Lucia namespace while making it optional (tsc will throw a Cannot find namespace error if you don't have it defined). It has to part of the next major release.

@pilcrowOnPaper pilcrowOnPaper added the v3 Version 3 target label Oct 10, 2023
@hobo1618
Copy link

@hdwatts Any chance you could link to a working t3-turbo + lucia example please? It seems like I have everything set up properly (the types propagate across packages, as per your example) but I get a 500 error whenever I try to create a user as per the nextJS + app directory tutorial on the site. I am pretty new to this, so a working example to look over would be great. I would also be happy to write it up as a guide if you're interested, @pilcrowOnPaper

@pilcrowOnPaper pilcrowOnPaper linked a pull request Nov 16, 2023 that will close this issue
6 tasks
@pilcrowOnPaper
Copy link
Member

Addressed in v3 (beta)

@pilcrowOnPaper pilcrowOnPaper removed a link to a pull request Jan 26, 2024
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request New feature requests v3 Version 3 target
Projects
None yet
Development

No branches or pull requests

5 participants