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 bug in POST endpoint where it would timeout #19

Merged
merged 5 commits into from
Mar 27, 2021
Merged

fix bug in POST endpoint where it would timeout #19

merged 5 commits into from
Mar 27, 2021

Conversation

JakeChampion
Copy link
Contributor

The toSearch function was never returning false because the find function returns null if it can not find a Todo record.

Adding a new function called exists which will always return a boolean. true if the record exists and false if it does not.

@lukeed
Copy link
Owner

lukeed commented Mar 27, 2021

Hey, thanks! But Model.find returns DB.read, which is Promise<T|false>. So result is going to be something truthy (the data) or false. The changes here are functionally the same thing

@JakeChampion
Copy link
Contributor Author

JakeChampion commented Mar 27, 2021

Hey, thanks! But Model.find returns DB.read, which is Promise<T|false>. So result is going to be something truthy (the data) or false. The changes here are functionally the same thing

Except in the scenario where no TODOs or users exist, then it gets stuck in an infinite loop and hits the cloudflare worker timeout scenario. When no TODOs exist, the DB.read looks to return null, which is falsey but the check in DB.until is doing a strict non-equality check for false --

while (exists !== false) {

This screenshot shows the user jake has no todos
image

This screenshot shows the timeout error that happens when a POST request is made to add a todo to jake
image

@JakeChampion
Copy link
Contributor Author

I added a console.log inside the DB.until toSearch function to show what the result from find is:

image

@lukeed
Copy link
Owner

lukeed commented Mar 27, 2021

Ah shoot, sorry – you're right. DB.read is supposed to be checking x != null ? x : false

src/kv.ts Outdated Show resolved Hide resolved
tsconfig.json Outdated Show resolved Hide resolved
/**
* Checks if a specifc `Todo` record already exists
*/
export async function exists(username: string, uid: string) {
Copy link
Owner

Choose a reason for hiding this comment

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

Please remove this new utility. It's redundant as find() serves the same purpose. This PR should only have a fix to DB.read

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've rebased all changes now into aca4266 (#19), it looks like typescript compiler is compiling into syntax which node12 does not support -- https://github.com/lukeed/worktop/runs/2209343024

@lukeed
Copy link
Owner

lukeed commented Mar 27, 2021

Ah, sorry. This is esbuild modernizing the output. Forgot it would "upgrade" syntax. I'll update that

Copy link
Owner

@lukeed lukeed left a comment

Choose a reason for hiding this comment

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

Sorry for the noise, thought I could get away with doing this via web editor only... nope 😅
Thanks!

@lukeed lukeed merged commit a024283 into lukeed:master Mar 27, 2021
@JakeChampion JakeChampion deleted the patch-1 branch March 27, 2021 21:49
@JakeChampion
Copy link
Contributor Author

JakeChampion commented Mar 27, 2021

Hurrah! I'm glad I reported this as I thought it was a bug in the example but it was really a little bug in worktop itself. Thank you for figuring that out and helping me with the pull-request

@lukeed
Copy link
Owner

lukeed commented Mar 27, 2021

Yeah :) All apps had .then(x => x || false) but I wanted to accommodate the fact that someone may want to store 0, so I changed it at the last minute & forgot about the null empty value 🤦

Thanks again, will go out in next release

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