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

fix(mikro-orm): re-enable tests #5316

Merged
merged 10 commits into from
Sep 21, 2022

Conversation

boredland
Copy link
Contributor

@boredland boredland commented Sep 9, 2022

swc/jest and decorators seems to be a hard nut to crack. But probably having reflection based metadata on our schema (and thus putting assumptions on the users setup) has not been a good idea to begin with. So I decided to follow the approach in an upstream PR and inserted reflection-less meta-data everywhere.

☕️ Reasoning

Aims to re-enable the tests, as I'd like to make some changes and need working regression tests.

closes #5286 as a side-effect.

🧢 Checklist

  • Documentation
  • Tests
  • Ready to be merged

@vercel
Copy link

vercel bot commented Sep 9, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment
Name Status Preview Updated
next-auth ⬜️ Ignored (Inspect) Sep 19, 2022 at 6:25PM (UTC)

@github-actions github-actions bot added adapters Changes related to the core code concerning database adapters mikro-orm @auth/mikro-orm-adapter labels Sep 9, 2022
@boredland boredland marked this pull request as draft September 9, 2022 09:20
@boredland boredland changed the title fix(mikro-orm): re-enable tests by setting up ts-jest fix(mikro-orm): re-enable tests Sep 9, 2022
@boredland boredland marked this pull request as ready for review September 9, 2022 10:11
@boredland boredland requested review from balazsorban44 and removed request for ubbe-xyz and ndom91 September 9, 2022 12:23
@boredland boredland force-pushed the chore/enable-mikro-orm-tests branch 2 times, most recently from d0ef0ca to a87b9db Compare September 9, 2022 13:28
@boredland boredland marked this pull request as draft September 9, 2022 13:54
@boredland

This comment was marked as outdated.

@boredland boredland marked this pull request as ready for review September 9, 2022 14:53
@boredland boredland force-pushed the chore/enable-mikro-orm-tests branch 2 times, most recently from 44a3271 to 64bd3b6 Compare September 9, 2022 15:02
@boredland
Copy link
Contributor Author

@balazsorban44 as we previously had a broken schema that was unusable for sso providers as I see it, we should make this a breaking change - users will need to run a migration that updates fields to their new types.

@boredland boredland force-pushed the chore/enable-mikro-orm-tests branch 5 times, most recently from b47141a to e74105a Compare September 12, 2022 10:35
@boredland

This comment was marked as outdated.

Copy link
Member

@ThangHuuVu ThangHuuVu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I skimmed the PR, it looks pretty good to me. Before we merge it, I'd like t ask for @ndom91's review

@boredland
Copy link
Contributor Author

Is there anything I can do to speed this up?

@boredland boredland force-pushed the chore/enable-mikro-orm-tests branch 2 times, most recently from b32e590 to 3627e89 Compare September 17, 2022 19:39
@ghost
Copy link

ghost commented Sep 19, 2022

@ndom91 and @balazsorban44 should I rebase this? don't want it to go stale again...

@boredland boredland force-pushed the chore/enable-mikro-orm-tests branch 2 times, most recently from a5726a7 to 1b60daa Compare September 19, 2022 18:23
@github-actions github-actions bot added the core Refers to `@auth/core` label Sep 19, 2022
While I have a working setup for tests with swc using 'next/jest',
I struggle getting a working setup here. So I installed ts-jest and some
missing peer dependencies.
BREAKING CHANGE:

apparently oauth providers were not working previously,
as two important fields had the wrong types.

this change will require users to run an appropriate migration.

closes nextauthjs#5286
BREAKING CHANGE

in mikro-orm prior to 4.2 collections emit a type `Collection<Session,
unknown>` this makes them essentially incompatible (read: unextendable)
by the newest mikro-orm versions. In newer versions this always should
be `Collection<Session, object>`, hence enforcing this for older mikro-
orm versions.
@github-actions github-actions bot removed the core Refers to `@auth/core` label Sep 19, 2022
@boredland
Copy link
Contributor Author

added another fix on top and updated some versions. perhaps you could have another look @ThangHuuVu ?

@ndom91
Copy link
Member

ndom91 commented Sep 21, 2022

Yeah this also looks good to me! Just approved another round of CI to double check the tests pass 👍

@ThangHuuVu
Copy link
Member

Cool, thank you @boredland @ndom91, merging it now 🙌

@ThangHuuVu ThangHuuVu merged commit 902bf92 into nextauthjs:main Sep 21, 2022
balazsorban44 added a commit that referenced this pull request Sep 25, 2022
@boredland boredland deleted the chore/enable-mikro-orm-tests branch October 9, 2022 10:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
adapters Changes related to the core code concerning database adapters mikro-orm @auth/mikro-orm-adapter
Projects
None yet
Development

Successfully merging this pull request may close these issues.

mikro-orm default entities generate invalid schema via SchemaGenerator
4 participants