fix: make peerDep @adonisjs/lucid as optional

Xstoudi commented 3 years ago

Proposed changes

Make @adonisjs/lucid peer dependency as optional.

Types of changes

[x] Bugfix (non-breaking change which fixes an issue)
[ ] New feature (non-breaking change which adds functionality)
[ ] Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist

Put an x in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.

[x] I have read the CONTRIBUTING doc
[x] Lint and unit tests pass locally with my changes
[ ] I have added tests that prove my fix is effective or that my feature works. (N/A ?)

Further comments

Without the change, installing @adonisjs/auth cause an error if @adonisjs/lucid is not installed.

npm ERR! code ERESOLVE
npm ERR! Found: @adonisjs/auth@6.0.1
npm ERR! node_modules/@adonisjs/auth
npm ERR!   @adonisjs/auth@"^7.0.0" from the root project
npm ERR!
npm ERR! Could not resolve dependency:
npm ERR! @adonisjs/auth@"^7.0.0" from the root project
npm ERR!
npm ERR!   peer @adonisjs/lucid@"^12.0.0" from @adonisjs/auth@7.0.0
npm ERR!   node_modules/@adonisjs/auth
npm ERR!     @adonisjs/auth@"^7.0.0" from the root project

targos commented 3 years ago

+1

An application may need to do authentication without having a database. In that case, it will have a custom auth provider, which doesn't need Lucid.

thetutlage commented 3 years ago

In theory yes. But I am not sure, if it is pragmatic or not. Even the node ace invoke command cannot configure the package for you, coz it forces you to choose between "Database" and "Lucid" for looking up users.

In short, I am trying to understand what are we trying to fix it and how many applications we have observed using this package without Lucid

Xstoudi commented 3 years ago

It's not only in theory, it's a practical problem we had while using @adonisjs/auth without Lucid.

I may have opened this PR too fast considering that we need to install Lucid anyway if we want to use it but this is what I suggest :

make the "Lucid" option install @adonisjs/lucid peer dep
add a "custom" choice to the node ace invoke (that doesn't install Lucid)

If this solution seems ok to you, I can push on this branch.

thetutlage commented 3 years ago

I am just trying to understand the actual issue. So please bear with me :)

If I get it right. The issue is, you received an error, because you were installing @adonisjs/auth before @adonisjs/lucid. But you will still use Lucid?
Or is it, you don't want to use Lucid at all and because of the peer dependency, you are not able to install auth package?

Xstoudi commented 3 years ago

No problem. We don't want to use Lucid at all and because of the peer dependency, we are not able to install auth package.

thetutlage commented 3 years ago

Okay that makes sense. We can mark the peer dependency optional.

Xstoudi commented 3 years ago

Is the PR I made enough or you need me to make another change?

thetutlage commented 3 years ago

It is good. Thanks 🙂

adonisjs / auth