-
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. |
24e93ee to
9dd77ac
Compare
|
@camillobruni can you take a look at this. Is this how the async runner is meant to be used? |
experimental/javascript-wc-indexeddb/src/storage/indexedDB-manager.js
Outdated
Show resolved
Hide resolved
| 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?
|
Updated results showed a big improvement when calling |
experimental/javascript-wc-indexeddb/dist/src/components/todo-item/todo-item.component.js
Show resolved
Hide resolved
resources/default-tests.mjs
Outdated
| 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?
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 think it's worth giving a shot to make it a "remote" workload. I'll look into how to use it with the async steps.
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.
@camillobruni @flashdesignory I modified this to use the remote runner, could you take a look. I had to manually copy some files from resources/todomvc/vanilla-examples/javascript-web-components/dist/src/speedometer-utils since I don't see those updates anywhere else than in that dist folder.
resources/default-tests.mjs
Outdated
| 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?
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.
You mean doing nothing hoping that the work happens between the measure step on one step and the measured time of the next? Since these are I/O operations I think properly waiting until they are done is the best approach.
experimental/javascript-wc-indexeddb/dist/components/todo-app/todo-app.component.js
Outdated
Show resolved
Hide resolved
experimental/javascript-wc-indexeddb/src/components/todo-app/todo-app.component.js
Outdated
Show resolved
Hide resolved
…he indexedDB version
d079142 to
9e2b659
Compare
| }, | ||
| { | ||
| name: "TodoMVC-WebComponents-IndexedDB", | ||
| url: "experimental/javascript-wc-indexeddb/dist/index.html?useAsyncSteps=true&storageType=vanilla", |
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'm unsure if using the query parameters here is the best way to flow down this information to the workload. I had to make some small modifications in suite-runner for this to work.
Results for the hosted prototype as of 10/24/2025