auth icon indicating copy to clipboard operation
auth copied to clipboard

Auth module strictly depends on Lucid

Open RomainLanz opened this issue 1 year ago • 7 comments

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 avatar Mar 09 '24 12:03 RomainLanz

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

Julien-R44 avatar Mar 09 '24 12:03 Julien-R44

Can't use import.meta.resolve?

targos avatar Mar 09 '24 12:03 targos

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'

thetutlage avatar Mar 11 '24 05:03 thetutlage

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.

RomainLanz avatar Mar 11 '24 07:03 RomainLanz

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

Julien-R44 avatar Mar 11 '24 09:03 Julien-R44

Ohh yeah, deprecation + new export will be the best

thetutlage avatar Mar 11 '24 12:03 thetutlage

@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 avatar Mar 11 '24 12:03 thetutlage

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

thetutlage avatar Apr 02 '24 09:04 thetutlage

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'

markgidman-rad avatar Apr 04 '24 20:04 markgidman-rad