Messaging - IDB Cleanup #523
Conversation
|
@jshcrowthe I can't add @pinarx as a reviewer to this PR for some reason. |
| return 'fcm_token_details_db'; | ||
| } | ||
| onDBUpgrade(db: IDBDatabase, evt: IDBVersionChangeEvent) { | ||
| if (evt.oldVersion < 1) { |
There was a problem hiding this comment.
this means it's brand new, right? can you add a quick comment?
| if (!db.objectStoreNames.contains(OLD_OBJECT_STORE_NAME)) { | ||
| // We found a database with the right name, but our expected object | ||
| // store isn't defined. | ||
| return; |
There was a problem hiding this comment.
this comment is confusing. So the v1 object store doesn't exist, and the new one is not defined yet? does this imply the old object store was dropped (maybe manually), and a new one isn't created yet?
There was a problem hiding this comment.
This is looking for the database named 'undefined', which could be used be created anyone, which if it is - we perhaps may not be the owner and therefore the expected object store wouldnt exist.
| return; | ||
| } | ||
|
|
||
| const transaction = db.transaction([OLD_OBJECT_STORE_NAME]); |
There was a problem hiding this comment.
can probably drop the brackets:
the following lines are equivalent:
var transaction = db.transaction(['my-store-name']);
var transaction = db.transaction('my-store-name');
| import IIDModel from '../models/iid-model'; | ||
|
|
||
| const OLD_DB_NAME = 'undefined'; | ||
| const OLD_OBJECT_STORE_NAME = 'fcm_token_object_Store'; |
There was a problem hiding this comment.
This code is purely cleaning up the old model (which shouldn't have been named 'undefined' but it was). So this, while related to the token-details-model, they are separate databases and I am just using the v1 -> v2 change to perform this one off clean up. So the version shouldn't every be included or needed here.
|
|
||
| const openCursorRequest: IDBRequest = objectStore.openCursor(); | ||
| openCursorRequest.onerror = event => { | ||
| // NOOP - Nothing we can do. |
There was a problem hiding this comment.
would it be helpful to log something to the console?
| } else { | ||
| // No more results - delete database if we are the only | ||
| // SDK using it | ||
| if (db.objectStoreNames.length === 1) { |
There was a problem hiding this comment.
can we just check if the name matches in that case? rather than checking the length?
|
Is it intentional that after this PR firebase.js is creating a new, empty IDB named "undefined" even when it doesn't exist, like when a new visitor comes or firebase.js 4.10.1 is deployed for the first time in a website? |
A few bugs have been highlighted from 4.9x => 4.10.0:
undefinedthe changes in 4.10 should have been fine, but because there is now a difference IDB name, the SDK is creating a new token, meaning end users can get two notifications - one for the old token and one for the new token.This PR will bump the version of the IDB database in the next release, and clean up any old IDB instances if they exist in the version upgrade step.