-
Notifications
You must be signed in to change notification settings - Fork 21
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
Photo upload #1755
Photo upload #1755
Conversation
Deployed to https://pr-1755.aam-digital.net/ |
This reverts commit 41e4c4c.
TODO:
|
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.
UX looks good to me 👍
- in offline mode photos are currently not cached/displayed
private reportProgress(message: string, obs: Observable<HttpEvent<any>>) { | ||
loadFile(entity: Entity, property: string): Observable<SafeUrl> { | ||
const path = `${entity.getId(true)}/${property}`; | ||
if (!this.cache[path]) { |
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.
what happens if the file is updated on another device and synced? Can/should we subscribe to entity updates somewhere to invalidate the cache in such a case?
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.
For new files (no previous value) this works as expected. For the other cases its a general caching question (also across reloads) when and how we get updates. At the moment the limit is the ngsw
config not so much this cache here.
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.
final approach ->staleWhileRevalidate (with no special in-memory cache handling).
This needs up to 2 reloads until the new photo is shown (1st to get the update, 2nd to serve it)
# Conflicts: # src/app/child-dev-project/children/child-block/child-block.component.ts # src/app/core/entity-components/entity-utils/dynamic-form-components/edit-component.ts # src/app/core/entity-components/entity-utils/dynamic-form-components/edit-photo/edit-photo.component.ts # src/app/features/file/edit-file/edit-file.component.ts
dc8a9f5
to
0975060
Compare
"urls": [ | ||
"**/child-photos/**" | ||
"**/app-attachments/**/**" | ||
], | ||
"cacheConfig": { | ||
"maxSize": 500, |
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 these values (size 500 max age 10 days) still make sense? This means users will have an old image for up to 10 days.
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.
if I understand the docs correctly, after remaining 10 days offline the image will not be displayed at all anymore ("considered invalid and eviced": https://angular.io/guide/service-worker-config#maxage)? In that sense, a big maxAge makes sense for offline support.
Maybe we should change this to the mentioned "staleWhileRevalidate" strategy though? In combination with our in-memory cache that could be a better fit?
d3212bb
to
d99dd93
Compare
d99dd93
to
f4861ed
Compare
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.
Offline photos also work now and things look good 💯
Only in offline mode a ton of errors and failed requests show in the console. I guess they don't end up in sentry (because they can't be sent without internet?) but for debugging this might pose some pain. Not sure whether we can do anything about it?
SonarCloud Quality Gate failed. 1 Bug No Coverage information |
🎉 This PR is included in version 3.20.0-master.3 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
🎉 This PR is included in version 3.20.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
see issue: #1569
Visible/Frontend Changes
Architectural/Backend Changes