-
Notifications
You must be signed in to change notification settings - Fork 88
Add TodoMVC with indexedDB storage #523
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
base: main
Are you sure you want to change the base?
Add TodoMVC with indexedDB storage #523
Conversation
✅ Deploy Preview for webkit-speedometer-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Hi, could you rebase on the latest main branch? I'd be up for a round of reviews today. |
On it, if you have to choose, please prioritize reviewing this workload's design doc instead of the code. |
This reverts commit 8dff6f2.
24e93ee to
9dd77ac
Compare
|
@camillobruni can you take a look at this. Is this how the async runner is meant to be used? |
| throw new Error("Database connection is not established"); | ||
|
|
||
| // Add todo item to IndexedDB | ||
| const transaction = this.db.transaction(this.storeName, "readwrite"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want a new transaction for every store? Here the 100 items are added synchronously and it would make sense to reuse the same transactions, but what does real web pages do?
…he indexedDB version
|
Updated results showed a big improvement when calling |
experimental/javascript-wc-indexeddb/dist/components/todo-item/todo-item.component.js
Show resolved
Hide resolved
| new BenchmarkTestStep( | ||
| "FinishAddingItemsToDB", | ||
| async (page) => { | ||
| await page.getGlobalVariable("addPromise"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know time is a bit tight – but I think this would be a very good use-case to create a fake "remote" workload where we drive the actions from the workload itself instead of trying to make everything go through the Page wrapper object.
Currently we don't have too many examples of this approach though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you just use page.call(awaitAdd) instead then we don't need the whole global variables helper.
WDYT?
| await page.getGlobalVariable("completePromise"); | ||
| }, | ||
| /* ignoreResult =*/ true | ||
| ), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
given this is experimental I'm kinda ok with it. But how high are the chances that we can hide work in the unmeasured commit-phases?
|
|
||
| moveToPreviousPage() { | ||
| // Skeleton implementation of previous page navigation | ||
| this.list.moveToPreviousPage().then(() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: why not using an async function here?
| this.#numberOfCompletedItems--; | ||
|
|
||
| this.list.toggleItem(event.detail.itemNumber, event.detail.completed); | ||
| this.update("toggle-item", event.detail.id); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this.update(...) takes no arguments – maybe I'm missing something here?
| import { getTodoText, defaultLanguage } from "./shared/translations.mjs"; | ||
| import { numberOfItemsToAdd } from "./shared/todomvc-utils.mjs"; | ||
|
|
||
| export const defaultSuites = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Going to land #519 so this will have to move to the experimental files – sorry for that.
Results for the hosted prototype as of 10/24/2025