Okay, think I got it and sounds good to me.
On Wed, Jun 18, 2014 at 3:48 AM, Javier Puerto <[email protected]> wrote: > I've explained my use case in the issue CB-6928 but as summary I'm creating > a simple rsync to keep the data in the device up to date. > > 2014-06-18 4:08 GMT+02:00 Ian Clelland <[email protected]>: > > > On Tue, Jun 17, 2014 at 9:34 PM, Andrew Grieve <[email protected]> > > wrote: > > > > > If it's cached... won't it exist? > > > > > > > Exactly this. A 304 request should only be received in response to a > > conditional GET request. There's generally no reason to send a > conditional > > GET unless you already have a cached copy of the file -- that is, unless > > you really don't care about its contents; only whether it's changed. But > if > > that's the case, then why are you using the file-transfer plugin? > > > > That's right. I use the file-transfer plugin because I see that it allows > custom headers so I thought that this way I can avoid an unnecessary HEAD > request first, just set the header and expect that the content is updated > if it's necessary or skipped in case that it's not. > > > > > > File-transfer is designed to be really simple. I think that we should > allow > > the developer to set If-Modified-Since headers any way that they like -- > or > > even If-None-Match, or crazier headers like If-Range, but we shouldn't > set > > those headers by default. Then, if a 304 is returned, then the right > thing > > to do is return *success* and not touch the file on disk at all. > > File-transfer has fulfilled its API: the file exists where it should. > > > > I'm agree. The patch I sent just skips the buffer copy if a 304 is detected > as response status. The problem is that if we are using a temporary file as > target, the plugin will return an invalid file entry. Adding an extra > parameter to communicate a cached response should do the job but for me it > looks too complicated, error has an status parameter in the callback and > conceptually the file-transfer didn't transferred anything so use the error > callback it's fine with me defining a new status for caching. > > > > > > I don't think that we should automatically set the If-Modified-Since > > header. My concern is that lots of other file operations could change > that > > date -- usually by modifying the file and setting it to *now* -- and in > > that case, you might never get a newer copy of the resource you've > > requested.'ve' > > > > The developer should be responsible to set the headers that he wants. For > my use case, the "rsync" updates the device and the application will just > consume the resources so no more modifications in FS. > > > > > > We could certainly add a flag to insert the header, based on the file's > > timestamp. Then a dev who knows the file hasn't been touched can set it. > > > > I think that anyone looking for something more complicated than this > should > > just use XHR and File, and manage the request and response headers > > themselves. > > > > > I don't think that my use case is complicated, I just expect that the > file-transfer update the file if it's necessary. Definitively the current > behaviour is wrong as doesn't makes sense to read a 304 response, it's > empty. Try to avoid an initial HEAD request it's just to save an extra > request that it's not necessary. Using different request means that in the > worst case, I will have to do two request per resource to update. > > > > Ian > > > > > > > > > > On Tue, Jun 17, 2014 at 11:56 AM, Javier Puerto <[email protected]> > > wrote: > > > > > > > I think it's better to use the error callback because for cached > > > resources > > > > doesn't makes sense to use the "Entry" as parameter as the target > will > > > not > > > > exists..There's no error but the file transfer was unable to download > > > > anything due to the 304 response so IMO the error callback could do > the > > > > job. > > > > > > > > > > > > 2014-06-17 16:27 GMT+02:00 Andrew Grieve <[email protected]>: > > > > > > > > > How about adding a second parameter to the callback? Android and > iOS > > > > > bridges both support this natively, and you can simulate it on > other > > > > > platforms by manually unpacking the parameters in your own > callback. > > > > > > > > > > > > > > > On Tue, Jun 17, 2014 at 4:18 AM, Javier Puerto <[email protected]> > > > > wrote: > > > > > > > > > > > 2014-06-16 17:01 GMT+02:00 Andrew Grieve <[email protected]>: > > > > > > > > > > > > > I think this behaviour has been around for a while, and makes > > sense > > > > in > > > > > > the > > > > > > > majority of cases. > > > > > > > > > > > > > > > > > > Yep, I did a "git blame" and this fragment of code was there from > > the > > > > > > beginning. > > > > > > > > > > > > > > > > > > > Best practice is to download to a temporary location, > > > > > > > and then upon success move the file to its final spot. > > > > > > > > > > > > > > > > > > > Yes, this is what I will have to do at the end. The thing is > that I > > > > want > > > > > to > > > > > > avoid a double step process. Anyway I'm still thinking that a > hard > > > > coded > > > > > > and undocumented behaviour is not a good practice neither, mostly > > > > because > > > > > > there's the tools for the developer to act as he needs with the > > > success > > > > > or > > > > > > error callback. It's about flexibility. > > > > > > > > > > > > > > > > > > > > > > > > > > That said, I think it'd be fine to add an option for not delete > > on > > > > > error. > > > > > > > > > > > > > > > > > > > It seems like the file transfer download is oriented to use a > > > temporary > > > > > > directory so it's fine for me as is (keep it simple). I think > that > > an > > > > > > explanation in the plugin documentation should be great so > everyone > > > can > > > > > > figure how to use the download. > > > > > > > > > > > > About the issue CB-6928, my patch it's not valid for me as I will > > use > > > > the > > > > > > temporary directory now and probably for everyone so I will have > to > > > > > cancel > > > > > > the pull request. The problem is still there but I wonder how can > > we > > > > fix > > > > > it > > > > > > if the success callback argument is an "Entry" object, the ideal > > > should > > > > > be > > > > > > to be able to communicate the caching status. I will change the > > patch > > > > to > > > > > > use the error callback to communicate the caching status, it's > not > > an > > > > > error > > > > > > but I don't see other way. I will add also the documentation for > > the > > > > new > > > > > > caching status code and open a new pull request. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Mon, Jun 16, 2014 at 5:39 AM, Javier Puerto < > > [email protected]> > > > > > > wrote: > > > > > > > > > > > > > > > Hi Cordova developers, > > > > > > > > > > > > > > > > I'm creating a system to download/update several resources > > from a > > > > > > server > > > > > > > to > > > > > > > > the device and I've observe a behaviour that breaks my use > > case. > > > > > > > > > > > > > > > > After fix the issue CB-6928, I'm able to download/update all > > the > > > > > > > resources > > > > > > > > without problems. My next test was to try to download the > > > resources > > > > > but > > > > > > > > with no server response (stopped). I've found that the plugin > > is > > > > > > deleting > > > > > > > > my target file silently because there's was an error. > > > > > > > > > > > > > > > > I think that the developer should be the responsible to > delete > > or > > > > > leave > > > > > > > the > > > > > > > > "corrupted" file because the file-transfer plugin already > > > > > communicates > > > > > > > the > > > > > > > > error, I don't think that the hardcoded behaviour is a good > > > > solution. > > > > > > > What > > > > > > > > do you think? > > > > > > > > I can open a new issue and provide the patch and test case > > > (Android > > > > > > only) > > > > > > > > > > > > > > > > Best regards. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
