Skip to content
This repository has been archived by the owner on Jan 3, 2023. It is now read-only.

Regression on image caching #60

Closed
Galadirith opened this issue Aug 7, 2015 · 6 comments
Closed

Regression on image caching #60

Galadirith opened this issue Aug 7, 2015 · 6 comments
Milestone

Comments

@Galadirith
Copy link
Collaborator

@leipert I like your changes moving as much logic as possible to the pathwatcher listener in 7c0949f but there is a regression on deleted images (which I should have added a spec for, my fault). Steps to reproduce:

  1. Preview an image
  2. Modify the image bumping query to ?v=2
  3. Delete the image setting query to ?v=deleted
  4. Restore image however it sets the query to ?v=1

Part of the problem is that iv > 0 evaluates as false if iv is deleted or renamed. But simply conditioning on that is likely to cause similar regressions after further delete restore cycles.

A possible design to fix would be to push the version to imgVersion[src][1] if the event is not change and restore that version when the image is restored, although I don't know which of the three events is triggered when an image is restored, I would guess change by the steps experienced above.

Thanks @leipert :D

@leipert
Copy link
Contributor

leipert commented Aug 7, 2015

Actually that is really what I wanted 💃
What about using unix timestamp instead of an integer

@Galadirith
Copy link
Collaborator Author

Oh of course my bad, there is nothing else that condition could be checking for, ok :D.

So the only problem then is that resetting the query to ?v=1 is that then uses the ?v=1 cache from before the image was deleted.

I like the unix timestamp idea a lot, solves the problem without adding any extra logic to resolveImagePaths(), great :D

@Galadirith
Copy link
Collaborator Author

Surprisingly the specs didn't pass for me @ 7c0949f. The issue seemed to be that on Windows when fs.renameSync() is invoked a rename event is fired followed immediately by a change event; I don't know weather its a different order on OSX or simply that only rename fires or some other reason. Again my fault for not having a spec to test renaming in the first place.

I think this can be fixed easily enough by decoupling the debounce from the listener and applying just to triggering a rerender.

@Galadirith
Copy link
Collaborator Author

@leipert let me know if you wanted me to handle these changes and if you wanted me to integrate the minor changes sitting on a486279 :D

@leipert
Copy link
Contributor

leipert commented Aug 8, 2015

#59 is merged, just needs #60 to be working really great.

If you want, do those changes!

@leipert leipert added this to the 1.7.0 milestone Aug 9, 2015
@Galadirith
Copy link
Collaborator Author

Great, will handle that shortly :D

leipert added a commit that referenced this issue Aug 9, 2015
Fixes #50 
Merged even if some specs are failing, as the failing specs are in the context of #60
Galadirith added a commit that referenced this issue Aug 9, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

2 participants