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

Auth module strictly depends on Lucid #236

Closed
RomainLanz opened this issue Mar 9, 2024 · 9 comments
Closed

Auth module strictly depends on Lucid #236

RomainLanz opened this issue Mar 9, 2024 · 9 comments
Assignees
Labels
Priority: High Look into this issue before picking up any new work Type: Bug The issue has indentified a bug

Comments

@RomainLanz
Copy link
Member

RomainLanz commented Mar 9, 2024

Hey there! 👋🏻

The @adonisj/auth module strictly depends on @adonisjs/lucid since we added the withAuthFinder mixins.

The mixin imports a Lucid decorator at its root:

import { beforeSave, type BaseModel } from '@adonisjs/lucid/orm'

Since we are exporting the mixin from the root of the package, we strictly depend on it and cannot use the Auth module without installing Lucid.

// index.ts
export { withAuthFinder } from './src/mixins/with_auth_finder.js'

We must find a way to remove this hard dependency without adding any breaking change to the current API.

@RomainLanz RomainLanz added Type: Bug The issue has indentified a bug Priority: High Look into this issue before picking up any new work labels Mar 9, 2024
@Julien-R44
Copy link
Member

I would suggest trying something like this :

// utils.ts
export function isModuleInstalled(moduleName: string) {
  const require = createRequire(import.meta.url)
  try {
    require.resolve(moduleName)
    return true
  } catch (error) {
    return false
  }
}

// index.ts of @adonisjs/auth
let withAuthFindder
if (isModuleInstalled('@adonisjs/lucid')) {
  withAuthFinder = await import('./with_auth_finder')
}

export { withAuthFinder }

this should work fine

@targos
Copy link
Member

targos commented Mar 9, 2024

Can't use import.meta.resolve?

@thetutlage
Copy link
Member

Should we implement the suggestion shared by @Julien-R44 or have a breaking change release that moves the import to a sub-module?

import { withAuthFinder } from '@adonisjs/auth/mixins'

@RomainLanz
Copy link
Member Author

Should we implement the suggestion shared by @Julien-R44 or have a breaking change release that moves the import to a sub-module?

import { withAuthFinder } from '@adonisjs/auth/mixins'

It feels better to have the withAuthFinder method in its own sub-module, but I believe we can implement @Julien-R44's suggestion to avoid a breaking change.

However, we keep in mind that this needs to be done in the next major release.

@Julien-R44
Copy link
Member

Julien-R44 commented Mar 11, 2024

i agree a breaking change just for that would be a bit annoying

I would say :

  • lets re-export withAuthFinder from the mixins submodule right now
  • lets also annotate the withAuthFinder exported from the root module with deprecated tag

so people can migrate in peace

@thetutlage
Copy link
Member

Ohh yeah, deprecation + new export will be the best

@thetutlage
Copy link
Member

@RomainLanz Can you do a PR for it and at the same time update docs to use new import path, so that atleast new apps are using the submodule.

@thetutlage
Copy link
Member

@markgidman-rad
Copy link

In case anyone comes here later, assuming you are using lucid, to adopt this change simply update

import { withAuthFinder } from '@adonisjs/auth'
to
import { withAuthFinder } from '@adonisjs/auth/mixins/lucid'

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority: High Look into this issue before picking up any new work Type: Bug The issue has indentified a bug
Projects
None yet
Development

No branches or pull requests

5 participants