Closed tomsaleeba closed 3 years ago
When it comes to dealing with photos that don't have a thumbnail as part of the EXIF, we have some options:
The third option is the best for UX. We need to benchmark some options and see how they perform. The options are:
Any solution that relies on using a canvas to do the resize might not be suitable. I don't think workers can use a canvas. They might be able to use an OffscreenCanvas but it's still experimental and Safari doesn't support it.
On the observation details page, I'm not sure if we can get away with showing the thumbnail or if we want the full size image to render. Perhaps we can have a lazy-load approach where we show the thumbnail and a button that will load the full-size photo. That way, we're less likely to be loading photos and if doing so causes issues, the user can just not do that.
Some thoughts on migrating. We need to move all the existing photos out into their own records, but if the user's phone is already full, we have to be able to read/write records without killing the browser tab.
If we no longer load photos, we'd be fine. But with the expectation that all photos are thumbnails, we plan on blindly loading them safely. Perhaps we should add a sanity check during obs record load to protect against:
According to page 27 (of the PDF, page 21 in the document) of the spec, the thumbnail will be less than 64kb:
...but maybe it's best to verify.
Back onto the topic, if we run the migration before we let anything else load, we might be safe with memory usage.
As for how to tell if we need to do a migration, perhaps we should adopt the approach like other DB migration software. Have a list of migrations and set flags when they've been applied.
Doing the migration on 20 test observations (generated from the admin screen) takes:
We're experiencing https://github.com/localForage/localForage/issues/910 in Safari.
Test process:
<wow-instance>/settings
, e.g. https://dev.app.wildorchidwatch.org/settings (you don't need to login)debug
level messagesError screenshot:
Test results:
Also seen this error on LambdaTest iOS 13.3 (Safari 13.1) simulator:
This is just after the page refresh when the migration starts to run.
When I change the code for https://github.com/localForage/localForage/blob/c1cc34f/dist/localforage.js#L1060 to:
transaction.onabort = function () {
console.warn('abort')
reject(req.transaction.error);
};
transaction.onerror = function () {
console.warn('error')
try {
var err = req.error ? req.error : req.transaction.error;
reject(err);
} catch (err2) {
reject(err2);
}
};
...so we stop masking the true error, we get:
Apparently this error can be triggered by:
new WeakSet().add(1);
When looking in the built code for our obs worker, the only mentions of a WeakSet I can find come from:
./node_modules/@sentry/utils/esm/memo.js
./node_modules/lodash/lodash.js
Lodash doesn't seem to use WeakSets internally.
I can't find any use of WeakSet in our main code:
yarn serve:debug
curl http://localhost:8081/js/app.js | grep -i weakset
curl http://localhost:8081/js/vendors.js | grep -i weakset
Currently we save a single record in IndexedDB that contains everything for an observation including photos. The IndexedDB best practices advise against doing that:
This means we need make the following changes: