-
-
Notifications
You must be signed in to change notification settings - Fork 52
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: add random number to db id seed #276
Conversation
@kettanaito Any chance of this change (or similar) going live? Seems like a pretty quick fix to make this whole library useable with vitest. |
This reverts commit 77d2263.
Apologies, apparently GitHub decided not to run CI for this pull request, making me believe it was safe to merge. It wasn't, this change breaks some of the Database functionality. It think it has to do with the I will revert this change. |
FYI I've been using this change with patch package for the last 5 months and it's working well for us. Let me know if there's anything I can do to help get some fix for the issue over the line. Is this package no longer actively maintained? We use it very heavily and were looking forward to it maturing with time! We may be able to contribute some amount of sponsorship if that would help. |
FWIW I also made this change locally and it completely fixes our issue as well. Wondering if there's a similar solution that addresses the issue you're seeing @kettanaito. Barring that, @themagickoala would you be willing to share a bit more about your |
Sure, I'll take a look on Monday and share. |
@themagickoala, thanks for the effort on this! It was my oversight not checking the CI status before merging (green on GitHub doesn't mean passing tests, apparently). I get reliably failing tests with the proposed changes. Regarding the library's state, I just created #285, please see. TL;DR the library is currently in maintenance mode while I'm working on finalizing its rewrite. Once the rewrite is done, I will be happy to continue with active maintenance. If you feel like you have an opportunity to sponsor MSW, any financial support is welcome. I'm close to finishing a major upgrade to MSW itself, and should have some time for Data later this fall/winter. This is actually one of the rewrites I really want to finish as it's been bugging me for years. |
You can reproduce the failing tests by: yarn install
yarn build
yarn test |
Released: v0.13.1 🎉This has been released in v0.13.1! Make sure to always update to the latest version ( Predictable release automation by @ossjs/release. |
Within your node modules folder, under I'm taking a look at the tests as @kettanaito mentioned above, so hopefully I can get this PR mergeable. |
Ok, I see the failing tests. Looks like the sync extension is always callled within factory? Is it possible to make this optional? The requirement for a reproducible db id is causing problems within Vitest tests, as with the current implementation the callOrder and callFrame are a match for each test suite. They run in parallel and the callOrder is reset within each suite, causing collisions of the MD5 hash. @kettanaito given that you're planning to rewrite this lib, you may not have an appetite for such a change. I'm happy to keep using my patch-package for my specific use case for now, but hopefully this requirement can be kept in mind for the rewrite, and db syncing can be opt-in (or at least opt-out)? |
@themagickoala Thanks so much for the step-by-step, that solved our issue perfectly! Appreciate you taking the time! |
Fixes #275