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

Add support for Microsoft SQL Server #514

Merged
merged 7 commits into from
Jul 31, 2020
Merged

Add support for Microsoft SQL Server #514

merged 7 commits into from
Jul 31, 2020

Conversation

iaincollins
Copy link
Member

@iaincollins iaincollins commented Jul 30, 2020

Testing out the new Microsoft SQL Server support added by @D10221

  • I'm happy this seems to work well and that it is ready for release.
  • I've tested positive and negative flows with a range of providers, including email.
  • The only changes to the original excellent Pull Request have been to the index (to allow multiple UNIQUE NULL values on email address, which is behaviour SQL Server handles slightly differently by design) and minor edits to documentation.

I propose we release this as v3.1 - along with the Basecamp provider PR in #511 and the support for entity hot reloading in #488, unless there are any objections or any other features or changes people would like to see make the cut?

Thanks again @D10221!

@vercel
Copy link

vercel bot commented Jul 30, 2020

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/nextauthjs/next-auth/fqmzzk6m5
✅ Preview: https://next-auth-git-feature-mssql.nextauthjs.vercel.app

@D10221
Copy link

D10221 commented Jul 30, 2020

As per Ms docs, snake_case should be the choice.
Another case for the snake-case, no pun intended, is it's case-insentive nature. Like css, urls. or cross-plaform file naming conventions

But I know by experience that is very common to use TitleCase, due it's strong connection to the .Net framework , c#, entity-framework, LinkToSql, Dapper, etc... due to tie to the POCOS (plain c# objects) m, Dtos or whatever you want to call them :).

Nevertheless I've seen Hungarian notation like naming everywhere too :(

Personally, I'll prefer names that can be mapped straight to the naming convention of language you are working on, just in case you need to 'raw query'.
As an example in Js I'll preffer camelCase, but I'm not offended snake_case ... I rather like it :), I feel is very neutral.

And again that might not be a problem with next-auth, or typeorm, as there is no need to have the models named exactly as they were typed on the database (correct me if wrong).
But with other mapping libraries like the plain 'mssql' or 'tedious' libs you get exactly what you typed on the queries and may complicate mappings if you have to share the database with those libs, or 'raw quieries' :)

IMMO as far you can change it to suit you needs it's all good. :)

... sorry it got a bit long

@iaincollins
Copy link
Member Author

@D10221 Thank you for the quick response, that's a really complete answer and I appreciate it!

I'm not familiar with mssql culture / conventions so wanted to avoid us doing something unconventional by default.

@vercel vercel bot temporarily deployed to Preview July 30, 2020 22:31 Inactive
@vercel vercel bot temporarily deployed to Preview July 30, 2020 22:36 Inactive
@vercel vercel bot temporarily deployed to Preview July 30, 2020 23:18 Inactive
@iaincollins iaincollins added the enhancement New feature or request label Jul 30, 2020
@iaincollins iaincollins marked this pull request as ready for review July 30, 2020 23:22
@vercel vercel bot temporarily deployed to Preview July 30, 2020 23:27 Inactive
@iaincollins iaincollins merged commit eb53219 into main Jul 31, 2020
@iaincollins iaincollins deleted the feature/mssql branch July 31, 2020 08:39
@iaincollins iaincollins changed the title Microsoft SQL Server Add support for Microsoft SQL Server Jul 31, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants