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: add random number to db id seed #276

Merged
merged 1 commit into from
Oct 7, 2023

Conversation

themagickoala
Copy link
Contributor

Fixes #275

@nico-martinucci
Copy link

@kettanaito Any chance of this change (or similar) going live? Seems like a pretty quick fix to make this whole library useable with vitest.

@kettanaito kettanaito changed the title feat: add random number to db id seed fix: add random number to db id seed Oct 7, 2023
@kettanaito kettanaito merged commit 77d2263 into mswjs:main Oct 7, 2023
kettanaito added a commit that referenced this pull request Oct 7, 2023
@kettanaito
Copy link
Member

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 seed becoming render on each import to Database since it's in the root scope of the module, making it impossible to reference an existing database. This is an eager assumption, I haven't looked into this library for some time now to say for certain.

I will revert this change.

@themagickoala
Copy link
Contributor Author

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.

@nico-martinucci
Copy link

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 patch-package fix for this issue? Would be a huge help to me, and maybe others who find this in the future.

@themagickoala
Copy link
Contributor Author

Sure, I'll take a look on Monday and share.

@kettanaito
Copy link
Member

@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.

@kettanaito
Copy link
Member

You can reproduce the failing tests by:

yarn install
yarn build
yarn test

@kettanaito
Copy link
Member

Released: v0.13.1 🎉

This has been released in v0.13.1!

Make sure to always update to the latest version (npm i @mswjs/data@latest) to get the newest features and bug fixes.


Predictable release automation by @ossjs/release.

@themagickoala
Copy link
Contributor Author

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 patch-package fix for this issue? Would be a huge help to me, and maybe others who find this in the future.

Within your node modules folder, under node_modules/@mswjs/data/lib/db/Database.js, add a variable called "seed" (I added this just above the definition of the Database class). Then inside the generateId class method, where it defines the salt variable, add this to the end: + "-" + Math.round(seed);. Finally, run npx patch-package @mswjs/data to generate a package patch, and add npx patch-package to your postinstall script in package.json (or add a postinstall script if you don't already have one).

I'm taking a look at the tests as @kettanaito mentioned above, so hopefully I can get this PR mergeable.

@themagickoala
Copy link
Contributor Author

You can reproduce the failing tests by:

yarn install
yarn build
yarn test

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)?

@nico-martinucci
Copy link

@themagickoala Thanks so much for the step-by-step, that solved our issue perfectly! Appreciate you taking the time!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Database ID is not unique when extracted to a setup helper file (using Vitest)
3 participants