search

olofd / react-native-photos-framework

A modern and comprehensive CameraRoll/iCloud-library-API for React Native 📸 📹
MIT License
220 stars 99 forks source link

Fix cleanCache-logic bug #4

Closed olofd closed 8 years ago

olofd commented 8 years ago

@athibaud Can you take a look at this? I removed the filter on type of 'function' (see my comment on your last PR). And I also found another bug. When running the cacheClean method at startup we also set up the Observer of library changes. This actually triggered the user gran-access dialog. So we clean the cache the first time anyone tries to run a function instead now. And we exclude auth-status-functions from this cleanCache-logic so that the user of the API can decide for them selfs when to trigger the dialog.

Have tested and it seems to work.

A future improvement would be to not set up the observation until the user explicitly is trying to watch a collection or assets. will take a look at that at a later point.

athibaud commented 8 years ago

indeed i noticed the grant dialog appeared on app startup but didn't realise cleanCache was responsible, thanks!

the code looks good to me. just tested in our app, seems to work fine.

A future improvement would be to not set up the observation until the user explicitly is trying to watch a collection or assets. will take a look at that at a later point.

yes this would be much better, and correct. our app crashes sometimes and haven't looked into the causes yet.. but having a change observer setup could be one of them. not sure yet if we'll have the time to delve into this in the coming weeks but please let us know if there is anything we can do :)

olofd commented 8 years ago

@athibaud I'll look into it asap. A crash does not seem good.. hmm.. please send me the stacktrace/try to create a repro if it happens again. Haven't had one single crash myself that was from the library code, so I'm very interested. Might be some configuration I haven't tried. Note: I have only ran this code on iOS 10. will do some testing on 8 and 9.

athibaud commented 8 years ago

@olofd we have no proof that the crash is caused by this module :) and if we ever find out that it is indeed, we will report it and do our best to fix it too, you can be sure of that ;)

all i'm saying is setting up an unnecessary change-observer could contribute to the crashing. so setting up that observer only when needed would be good future change indeed! :)

we have ios9 devices and we'll report/fix issues we encounter. cannot help for ios8 though :/

thanks again for the hard work! extremely appreciated!