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

SvelteKit demo app 'todos' failing on delete #1564

Closed
JBusillo opened this issue May 26, 2021 · 14 comments · Fixed by #6979
Closed

SvelteKit demo app 'todos' failing on delete #1564

JBusillo opened this issue May 26, 2021 · 14 comments · Fixed by #6979
Labels
Milestone

Comments

@JBusillo
Copy link
Contributor

Describe the bug
The SvelteKit demo app seems to have a bug when deleting 'todos'. It occurs when multiple todos are added and subsequently deleted.

Logs

--- Browser log
Failed to load resource: the server responded with a status of 500 (Internal Server Error)
{"status":500,"message":"Error deleting todo"}

Server log has no error messages

To Reproduce
  • Create a new SvelteKit demo app (npm init svelte@next app-dir), accepting all the defaults
  • cd app-dir
  • npm i
  • npm run dev -- --open
  • Open devtools

Perform the following steps (You may experience the error prior to completing all steps):

  • Navigate to TODOS, enter 3 TODOS.
  • Delete first TODO
  • Delete last TODO
  • Delete remaining TODO
  • Enter 3 TODOs again
  • Delete first TODO
  • Delete last TODO

Expected behavior
TODO's are deleted when requested

Diagnostics
System:
    OS: Linux 5.8 Ubuntu 20.04.2 LTS (Focal Fossa)
    CPU: (6) x64 Intel(R) Core(TM) i5-8400 CPU @ 2.80GHz
    Memory: 25.99 GB / 31.04 GB
    Container: Yes
    Shell: 5.0.17 - /bin/bash
  Binaries:
    Node: 14.17.0 - /usr/bin/node
    Yarn: 1.22.10 - /usr/local/bin/yarn
    npm: 7.12.0 - /usr/local/bin/npm
  Browsers:
    Brave Browser: 90.1.24.85
    Chrome: 90.0.4430.212
    Firefox: 88.0.1
  npmPackages:
    @sveltejs/kit: next => 1.0.0-next.109 
    svelte: ^3.34.0 => 3.38.2 

using Chrome (also fails using Firefox)

no adapter specified

Severity
Not severe. It may affect user perception of stability when evaluating SvelteKit.

Additional context
Failure is also present when running on Windows.

@benmccann benmccann added the bug Something isn't working label May 27, 2021
@benmccann benmccann added this to the 1.0 milestone May 27, 2021
@jesslawnz
Copy link

jesslawnz commented Jun 15, 2021

Had some difficulty reproducing this bug until I double clicked the delete icon. Able to reproduce it with a single item.

  • Create todo Item
  • double click delete icon
  • error occurs.

Found another similar bug in the TODO page:

  • Focus on input '+tap to add a todo'
  • Type 1 {enter} 2 {enter} 3 {enter} 4 {enter} 5 {enter} etc. as fast as possible
  • Expect: sequential TODO entries 1..5
  • Actual count and content of TODO entries is unpredictable.

@twobitfool
Copy link

I think the problem lies with the API server: https://api.svelte.dev/

We can see the issue by using curl to create 3 todo items, and then refetch them.
USER_ID=`uuidgen | tr '[:upper:]' '[:lower:]'`

echo "Adding 3 Tasks..."
curl -sH "Content-Type: application/json" -d '{"text":"First Todo"}'           "https://api.svelte.dev/todos/$USER_ID" | jq "."
curl -sH "Content-Type: application/json" -d '{"text":"Second Todo"}'          "https://api.svelte.dev/todos/$USER_ID" | jq "."
curl -sH "Content-Type: application/json" -d '{"text":"Third and Final Todo"}' "https://api.svelte.dev/todos/$USER_ID" | jq "."

sleep 1
echo "\nLoading Tasks..."
curl -sH "Content-Type: application/json" "https://api.svelte.dev/todos/$USER_ID" | jq ".[] | .text"

sleep 1
echo "\nLoading Tasks (Again)..."
curl -sH "Content-Type: application/json" "https://api.svelte.dev/todos/$USER_ID" | jq ".[] | .text"

sleep 1
echo "\nLoading Tasks (One More Time).."
curl -sH "Content-Type: application/json" "https://api.svelte.dev/todos/$USER_ID" | jq ".[] | .text"
When refetching the data, the API results are unpredictable.
Adding 3 Tasks...
{
  "uid": "748878bd1fd49332bfc648b63b2a2f713314",
  "created_at": 1624943002911,
  "text": "First Todo",
  "done": false
}
{
  "uid": "ec9cc1bb1329503b4abc2704ecc087008a25",
  "created_at": 1624943003178,
  "text": "Second Todo",
  "done": false
}
{
  "uid": "48221085a19461deb371ba466aea6301a0c5",
  "created_at": 1624943003569,
  "text": "Third and Final Todo",
  "done": false
}

Loading Tasks...
"First Todo"
"Second Todo"

Loading Tasks (Again)...
"First Todo"
"Second Todo"
"Third and Final Todo"                # <- The 3rd todo shows up here, but not in the previous or next request

Loading Tasks (One More Time)..
"First Todo"
"Second Todo"
Fetch All: [Todo 1, Todo 2]           # ❌  Wrong
Fetch All: [Todo 1, Todo 2, Todo 3]   # ✅  Correct
Fetch All: [Todo 1, Todo 2]           # ❌  Wrong

Looks like an "eventual consistency" issue with the API storage back-end.


Like a lot of people, I initially assumed that something was broken with SvelteKit. Might be worth changing the sample app to use in-memory (or local file) storage 🤷

@lunacd
Copy link
Contributor

lunacd commented Aug 10, 2021

I also feel like a local store (file or sqlite) might be desirable. It can also demonstrate the ability for SvelteKit to function as a HTTP server (I think it already does in the current form but it's still ultimately relying on some external HTTP service).

If people think that is a desirable change, I can help to implement that.

@ric2b
Copy link
Contributor

ric2b commented Aug 12, 2021

Opened the PR above, which simply disables the delete button while the delete request is in flight. Should get rid of this issue.

@twobitfool
Copy link

twobitfool commented Aug 12, 2021

Please reopen this issue...

PR 2172 is great, because it avoids a double-submit issue, but it does not fix the root problem. The root problem is with the (eventually) consistency of the API (api.svelte.dev).

The bug can occur when clicking the delete button just once. (Note: Very tricky, because it doesn't fail consistently.)

Method 1 (for triggering bug):

  1. Open web inspector
  2. Create a few todos
  3. Delete a todo
  4. Goto step 2, and repeat, until you see the bug

delete-bug


Method 2 (for triggering bug):

  1. Create several todos
  2. Delete a few todos
  3. Reload the page a few times
  4. Deleted todos will reappear & disappear on a subsequent reloads

Method 3 (for triggering bug):

  1. Run the curl script from my comment above.

This demonstrates the issue, by creating a few todos and reloading them three times.


To be clear: This is not a bug in the demo app, and it's not a bug in SvelteKit. This is a bug with the API server that is storing the todos. But it certainly needs to be addressed, because it makes SvelteKit look broken.

FWIW: Method 2 and 3 were the most reliable ways to reproduce the bug -- for me at least 🤷

@ignatiusmb
Copy link
Member

Thanks for the investigation @twobitfool! Perhaps it's better if we move this to https://github.com/sveltejs/api.svelte.dev then

@ignatiusmb ignatiusmb reopened this Aug 12, 2021
@twobitfool
Copy link

@ignatiusmb -- Yeah. That's an option.

...or option 2... We tweak the demo app to use local (or in-memory) storage, which would:

  • resolve this issue
  • remove the dependency on the API server
  • allow the demo to run offline
  • make the app lighting quick

@benmccann
Copy link
Member

Some notes from when we created the demo:

We have deployed versions of the demo apps (example) so wanted a real backend

Vercel was doing some kind of caching that was originally causing issues on the Vercel deployed version. I think that was possibly turned off and wouldn't explain any issues occurring locally

KV has a limit of 1 write per second per key. This app has all user todos living under todolist__{userid} key. The way to get around this is to do todolist__(userid)__(todoid) entries. And then the GET /todos is a KV.list for the todolist__(userid) prefix

If consistency is an issue, Durable Objects or Fauna would be strongly consistent (perhaps higher latency as a result)

@ric2b
Copy link
Contributor

ric2b commented Aug 13, 2021

@twobitfool Yeah, looks like the API itself has some weird consistency issues.

This seems to be about just that: sveltejs/api.svelte.dev#8

@benmccann Can you elaborate on what you mean by this?

so wanted a real backend

What is the benefit vs using local storage? Is it because it's an example that new users might want to look at to learn how to interact with API's or something else?

@sugihartolim
Copy link

Please give this issue a higher priority, the todos part of the demo has been broken since basically forever. First impression matters.

I second @twobitfool 's suggestion above to use local or in-memory storage.

@dummdidumm
Copy link
Member

The reason localStorage or anything JavaScript-in-the-browser-related isn't used is to show progressive enhancement capabilities: The TODO list functions even if you block JavaScript from executing on the page. Adding something browser-JavaScript-related would make that impossible.

@twobitfool
Copy link

@dummdidumm Agreed. It shouldn't be in-browser.

I should have been more clear before... When I mentioned memory or local storage, I didn't mean browser memory or localStorage. I was thinking of something on the "server" side (in the api routes) -- either an in-memory store or some simple serialization to disk. No client side JS required.

@denniske
Copy link

I was checking out sveltekit today and the laggy experience of the todos page put me off. After changing it to an in memory store in /todos/index.ts the response times of the api requests went from 300+ ms to ~5ms. I think this should be the default experience for new sveltekit users.

If the external api todos page is needed for showing features of sveltekit, I would suggest to make two todos pages (one with in memory and one with external api).

@hb0nes
Copy link

hb0nes commented Mar 14, 2022

I just wanted to comment here that I almost quit investing more time into SvelteKit because the Todo's really seem slow and wonky.

I can just click randomly on the Todo's until it 'breaks'.
When it breaks, it will check the Todo but then after a second or more, change its mind again.

I don't know, it's just very bad/wonky UX.

Seeing as it's one of the first thing new devs picking up Svelte will see, this is bad news.

@benmccann benmccann added the p2-nice-to-have SvelteKit cannot be used by a small number of people, quality of life improvements, etc. label Mar 17, 2022
@benmccann benmccann removed the p2-nice-to-have SvelteKit cannot be used by a small number of people, quality of life improvements, etc. label Jul 19, 2022
@Rich-Harris Rich-Harris mentioned this issue Sep 22, 2022
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.