-
Notifications
You must be signed in to change notification settings - Fork 804
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
DbWrapper deprecation #2838
DbWrapper deprecation #2838
Conversation
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.
Thanks! This all looks a lot cleaner, and I'm glad we were able to drop so much old code.
I just had the one small comment inline.
@@ -76,7 +78,9 @@ class CacheTimestampsModel { | |||
|
|||
// Previous versions of `workbox-expiration` used `this._cacheName` | |||
// as the IDBDatabase name. | |||
deleteDatabase(this._cacheName); | |||
if (this?._cacheName) { |
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 curious as to why it's this?
instead of this
. When would this
not be defined?
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.
Glad you asked, I meant to ask you about that, we call the method to upgradeDB in tests, for example: https://github.com/GoogleChrome/workbox/blob/v6/test/workbox-expiration/sw/test-CacheTimestampsModel.mjs#L16, and that is when this
would not be defined. I am not sure if this is still the correct way to do this tests, because the test would be the one creating the IDB because it is opened before each test, maybe we should refactor the tests to open the DB only in the test cases that needs it and only by passing the name?
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.
Hmmm, that's interesting.
I don't think most of the tests end up ever triggering the IDB upgrade flow, do they? Can we just leave out onupgradeneeded
from when we call openDB()
in our tests?
I think would be cleaner than having a situation in which we need to use this?
in the non-test code.
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.
Turns out my approach also wouldn't work, so I sort of separated the dbupgrade and the deleting of old dbs and that seems to work, let me know what you think. Thank you!
@@ -76,7 +78,9 @@ class CacheTimestampsModel { | |||
|
|||
// Previous versions of `workbox-expiration` used `this._cacheName` | |||
// as the IDBDatabase name. | |||
deleteDatabase(this._cacheName); | |||
if (this?._cacheName) { |
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.
Hmmm, that's interesting.
I don't think most of the tests end up ever triggering the IDB upgrade flow, do they? Can we just leave out onupgradeneeded
from when we call openDB()
in our tests?
I think would be cleaner than having a situation in which we need to use this?
in the non-test code.
@tropicadri pointed out that As discussed, moving the call to |
Fixes #2801
Changes in workbox-expiration to use idb and deleting DBWrapper.