adonisjs / auth

Official Authentication package for AdonisJS
https://docs.adonisjs.com/guides/auth/introduction
MIT License
187 stars 65 forks source link

Auth module strictly depends on Lucid #236

Closed RomainLanz closed 3 months ago

RomainLanz commented 4 months ago

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.

Julien-R44 commented 4 months ago

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 commented 4 months ago

Can't use import.meta.resolve?

thetutlage commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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

I would say :

so people can migrate in peace

thetutlage commented 4 months ago

Ohh yeah, deprecation + new export will be the best

thetutlage commented 4 months ago

@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 commented 3 months ago

Released. https://github.com/adonisjs/auth/releases/tag/v9.2.0

markgidman-rad commented 3 months ago

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'