-
-
Notifications
You must be signed in to change notification settings - Fork 88
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you so much for submitting this! I had some comments, but all in all it looks great so far!
PS.: If you are into TypeScript (totally fine if you are not interested), we now have an Adapter
interface, that could help you type the whole adapter, by applying the interface on the Adapter function.
"use strict"; | ||
|
||
class UnknownError extends Error { | ||
constructor(message) { | ||
super(message); | ||
this.name = 'UnknownError'; | ||
this.message = message; | ||
} | ||
|
||
toJSON() { | ||
return { | ||
error: { | ||
name: this.name, | ||
message: this.message | ||
} | ||
}; | ||
} | ||
|
||
} | ||
|
||
class CreateUserError extends UnknownError { | ||
constructor(message) { | ||
super(message); | ||
this.name = 'CreateUserError'; | ||
this.message = message; | ||
} | ||
|
||
} | ||
|
||
class AccountNotLinkedError extends UnknownError { | ||
constructor(message) { | ||
super(message); | ||
this.name = 'AccountNotLinkedError'; | ||
this.message = message; | ||
} | ||
|
||
} | ||
|
||
module.exports = { | ||
UnknownError, | ||
CreateUserError, | ||
AccountNotLinkedError | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That file can be deleted. You can import all these (and more!) from next-auth/errors
now! 🎉
See https://github.com/nextauthjs/next-auth/blob/main/src/lib/errors.js
(Note. We are also trying to make error handling automatic, so in the future, you may not even need these, but for now, it's fine if you use them wherever possible and fit. You could see that Prisma adapter for reference how and where it uses these Errors: https://github.com/nextauthjs/adapters/blob/canary/packages/prisma/src/index.ts. This will help the users catch their errors much more easily)
"use strict"; | ||
|
||
Object.defineProperty(exports, "__esModule", { | ||
value: true | ||
}); | ||
exports.default = void 0; | ||
var logger = { | ||
error: function error(errorCode) { | ||
for (var _len = arguments.length, text = new Array(_len > 1 ? _len - 1 : 0), _key = 1; _key < _len; _key++) { | ||
text[_key - 1] = arguments[_key]; | ||
} | ||
|
||
if (!console) { | ||
return; | ||
} | ||
|
||
if (text && text.length <= 1) { | ||
text = text[0] || ''; | ||
} | ||
|
||
console.error("[next-auth][error][".concat(errorCode.toLowerCase(), "]"), text, "\nhttps://next-auth.js.org/errors#".concat(errorCode.toLowerCase())); | ||
}, | ||
warn: function warn(warnCode) { | ||
for (var _len2 = arguments.length, text = new Array(_len2 > 1 ? _len2 - 1 : 0), _key2 = 1; _key2 < _len2; _key2++) { | ||
text[_key2 - 1] = arguments[_key2]; | ||
} | ||
|
||
if (!console) { | ||
return; | ||
} | ||
|
||
if (text && text.length <= 1) { | ||
text = text[0] || ''; | ||
} | ||
|
||
console.warn("[next-auth][warn][".concat(warnCode.toLowerCase(), "]"), text, "\nhttps://next-auth.js.org/warning#".concat(warnCode.toLowerCase())); | ||
}, | ||
debug: function debug(debugCode) { | ||
for (var _len3 = arguments.length, text = new Array(_len3 > 1 ? _len3 - 1 : 0), _key3 = 1; _key3 < _len3; _key3++) { | ||
text[_key3 - 1] = arguments[_key3]; | ||
} | ||
|
||
if (!console) { | ||
return; | ||
} | ||
|
||
if (text && text.length <= 1) { | ||
text = text[0] || ''; | ||
} | ||
|
||
if (process && process.env && process.env._NEXTAUTH_DEBUG) { | ||
console.log("[next-auth][debug][".concat(debugCode.toLowerCase(), "]"), text); | ||
} | ||
} | ||
}; | ||
var _default = logger; | ||
exports.default = _default; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logger instance is available on appOptions.logger
. You get appOptions
from getAdapter
.
Again, you can have a look at the Prisma adapter:
adapters/packages/prisma/src/index.ts
Lines 87 to 88 in edff780
async getAdapter(appOptions: AppOptions) { | |
const logger = appOptions.logger ?? console |
Co-authored-by: Balázs Orbán <info@balazsorban.com>
Co-authored-by: Balázs Orbán <info@balazsorban.com>
Co-authored-by: Balázs Orbán <info@balazsorban.com>
Co-authored-by: Balázs Orbán <info@balazsorban.com>
Co-authored-by: Balázs Orbán <info@balazsorban.com>
Co-authored-by: Balázs Orbán <info@balazsorban.com>
import "regenerator-runtime/runtime"; | ||
import { createHash, randomBytes } from "crypto"; | ||
|
||
import { CreateUserError } from "./lib/errors"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
import { CreateUserError } from "./lib/errors"; | |
import { CreateUserError } from "next-auth/errors"; |
These are included in the base library now. check out the content of the errors module, there are more type of errors as well. You can delete the ./lib/errors
file now
import { createHash, randomBytes } from "crypto"; | ||
|
||
import { CreateUserError } from "./lib/errors"; | ||
import logger from "./lib/logger"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
import logger from "./lib/logger"; |
You will find this in appOptions
in getAdapter
. You can remove ./lib/logger
Co-authored-by: Balázs Orbán <info@balazsorban.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work, would be great to get this in.
packages/sequelize/LICENSE
Outdated
AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER | ||
LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, | ||
OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE | ||
SOFTWARE. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This license isn't necessary given there's already one for the repo.
"@babel/core": "^7.12.10", | ||
"@babel/preset-env": "^7.12.11" | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should have a peer dependency on sequelize, right?
@EXIT-ALAMERY we now have official TS types for the adapters as well as a test suite all adapters should pass. Maybe these can help you finalize this adapter. If you're still interested in getting this sequelize adapter in, I'd suggest checking out the suggestions made above and then maybe checking out the new Looking forward to this! |
@ndom91 I have talked to @EXIT-ALAMERY and unfortunately he doesn't have the opportunity to finish this up, so I am going to take this PR once I have the time, and credit them as a contributor. |
Co-authored-by: Luke Jones <luke-j@users.noreply.github.com>
Co-authored-by: Luke Jones <luke-j@users.noreply.github.com>
Co-authored-by: Luke Jones <luke-j@users.noreply.github.com>
Co-authored-by: Luke Jones <luke-j@users.noreply.github.com>
Co-authored-by: Luke Jones <luke-j@users.noreply.github.com>
Co-authored-by: Luke Jones <luke-j@users.noreply.github.com>
Co-authored-by: Luke Jones <luke-j@users.noreply.github.com>
Co-authored-by: Luke Jones <luke-j@users.noreply.github.com>
Hey guys this is looking awesome! If you can add the basic tests to this package i'd be comfortable merging and releasing a canary version! See:
For an ORM like this where users can put a few types of different DBs underneath, ideally we'd like to have atleast 2+ (for example mysql + postgres) tested. But if we get one in and working I think that'd be a good enough starting point to release a canary version. |
Awesome thanks a lot! |
Tests for MySQL and Postgresql are in. Thanks guys! Is this alright to merge from your perspective? Btw you still need to add sequelize to the adapter matrix in adapters/.github/workflows/canary.yml Lines 33 to 40 in 468bd5e
Thanks! |
Hey guys I was taking a look at the tests, they don't seem to pass in CI and I couldn't get them to run locally either, but that makes sense since yuo don't have any script or any way to setup the db in a CI environment. The We need to come up with a shell script to automate that whole process in CI for both mysql and postgres. I'll try to push something here shortly 👍 Otherwise looks good so far! |
So I cleaned up some of the default usernames / etc to make them match to what we usually use. Next, I added some npm scripts for each mysql and postgres to execute a bash script to setup the respective containers and execute jest tests on them once they're up. Also none of the db drivers were listed as peerDeps or anything like that, so I added them as optional peerDeps for mysql and postgres. Both test (mysql + postgres) pass now locally for me, starting from 0, building the containers, running migrations, running tests, etc. However on both tests I still have a ton of console.log output from what looks like the sequelize adapter itself, but I couldn't find any What do you think? EDIT: Okay so while it works locally, github actions still seems to not like something. I'd bet its somethign to do with missing types. Have you guys seen |
The SQL queries you see in the console get generated by the sequelize library itself so, you can disable them by setting Also, I've seen the sequelize-typescript module but, the sequelize team doesn't officially maintain it. So, I decided not to implement it but, if you believe that proceeding with it would suffice the needs of this project, then I'll take a thorough look. Thanks! |
I only mention it because the tests seem to be failing due to missing types. I did some testing with manually declare a dummy sequelize module, but that didn't seem to help.. @lluia any idea whats missing here? Checkout the latest failing test. |
We can try adding sequelize as a dependency and see if it works because I created full-fledged sequelize models specifically for the tests, so they're trying to import sequelize from the node modules, which doesn't exist. |
Hey @EXIT-ALAMERY, we've done a major rewrite of the adapter API to simplify things significantly. Are you still interested in getting sequelize over the finish line? You can checkout the changes here: nextauthjs/next-auth#2361. As well as the migration @balazsorban44 did of our prisma adapter here: #171 and of fauna here: #182. Also we would really appreciate it if you could make the PR against We're also debating whether to replace |
That sounds great and, I wish all the best to nextauthjs, but, unfortunately, I am no longer interested in the sequelize adapter. Thank you for your patience and support. |
no problem! we might reuse your code when we get some time to add a sequelize adapter! thank you! |
Added the adapter for Sequelize ORM.