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

Drop p-limit internally #8809

Merged
merged 9 commits into from Feb 14, 2024
Merged

Drop p-limit internally #8809

merged 9 commits into from Feb 14, 2024

Conversation

dcousens
Copy link
Member

This pull request should remove the use of p-limit internally for limiting Prisma engine operations.
prisma/prisma#2955 has been resolved, and - at least as documented - this was the only reason this approach was taken.

@dcousens dcousens force-pushed the less-plimit branch 2 times, most recently from f8bd14a to c427408 Compare September 18, 2023 03:24
@codesandbox-ci
Copy link

codesandbox-ci bot commented Sep 18, 2023

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 85b2a1d:

Sandbox Source
@keystone-6/sandbox Configuration

@dcousens dcousens force-pushed the less-plimit branch 2 times, most recently from c6501c3 to 336db68 Compare December 4, 2023 05:12
@dcousens
Copy link
Member Author

dcousens commented Dec 4, 2023

Although this pull request removes the internal p-limit calls, and the aforementioned issue prisma/prisma#2955 has been closed, unfortunately the problems with sqlite don't end with that.

As reported in the following issues to prisma:

To prevent broken behaviour, especially in our tests, we need Prisma to internally queue the sqlite "connections".
The existing p-limit implementation used a global p-limit that prevented many tests running at the same time, this removes that restriction by ensuring the tests do not need need to run sequentially and instead relying on connection_limit=1 to prevent connection issues.

@dcousens dcousens force-pushed the less-plimit branch 3 times, most recently from 9c55746 to 8ba1162 Compare February 12, 2024 06:30
@dcousens dcousens self-assigned this Feb 12, 2024
@dcousens dcousens force-pushed the less-plimit branch 3 times, most recently from 81d7401 to 4c00768 Compare February 13, 2024 02:10
if (x === true) return 'True'
if (x === false) return 'False'
return 'Undefined'
if (x === true) return 'T'
Copy link
Member Author

@dcousens dcousens Feb 13, 2024

Choose a reason for hiding this comment

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

Shortened the strings to work within database limitations

async findMany () {
return [{ id: 'mock' }]
}
}
Copy link
Member Author

Choose a reason for hiding this comment

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

This is incompatible with how we are using Prisma now, and probably Prisma 5 too

@dcousens dcousens force-pushed the less-plimit branch 7 times, most recently from 9b3fa77 to 33eaa7e Compare February 13, 2024 11:44
for (const create of [undefined, 'create']) {
for (const update of [undefined, 'update']) {
for (const _delete of [undefined, 'delete']) {
const omit = [query, create, update, _delete].filter(x => x) as ListConfig['omit']
Copy link
Member Author

Choose a reason for hiding this comment

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

This was completely broken

@dcousens dcousens merged commit c0dacbf into main Feb 14, 2024
58 checks passed
@dcousens dcousens deleted the less-plimit branch February 14, 2024 03:34
@dcousens dcousens mentioned this pull request Mar 28, 2024
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.

None yet

2 participants