phonegap / phonegap-plugin-contentsync

Download and cache remotely hosted content
Apache License 2.0
206 stars 98 forks source link

Just updated - 404 succeeds instead of failing #86

Closed kaansoral closed 8 years ago

kaansoral commented 8 years ago

So I have a local testing setup, mostly the zip doesn't exist on the local setup, so when contentsync fails, the failovers kick in

Yet with a plugin update, the "complete" callback is called with "status":3 and the localPath directs to a ".zip" that doesn't exist (instead of being a folder)

Am I missing something or is this a bug?

macdonst commented 8 years ago

@kaansoral what platform, iOS/Android/Windows? It'll just help me track it down.

kaansoral commented 8 years ago

iOS on an iPod, Android seems to work

macdonst commented 8 years ago

@kaansoral can you give me an example url? What I'm seeing with the latest from master is 404's will error out but sites that redirect to 404 pages can cause problems.

kaansoral commented 8 years ago

I'm using AppEngine SDK at development, from the browser, it loads an empty page and 404 code from the network

I also check the logs and verify the 404 there

This might be one example of a 404: http://www.kaansoral.com/js/mobile.zip (This is production, not SDK)

This used to work flawlessly by the way, also the returned localPath that directs to a .zip that doesn't exist is interesting

macdonst commented 8 years ago

@kaansoral grab the latest from master and test as I can no longer reproduce your problem. Probably fixed by the change for #87

kaansoral commented 8 years ago

Using this: "cordova plugin add https://github.com/phonegap/phonegap-plugin-contentsync.git"

Tested it an hour ago, same issue, applied a ghetto fix that checks whether the path ends with ".zip" and manually calls the error method

macdonst commented 8 years ago

@kaansoral you are checking the src property to see if it ends in .zip?

kaansoral commented 8 years ago
    var error_f=function(e) {
        if(!retry && !fetch_libraries_completed) setTimeout(function(){fetch_libraries(retry+1);},3000);
        else
        {
            if(!fetch_libraries_completed) fetch_libraries_fallback();
        }
    };
    sync.on('error',error_f);
    sync.on('complete', function(data) {
        if(DEBUG && !retry) alert(JSON.stringify(data));
        if(endsWith(data.localPath,".zip")) { return error_f(); } // manual override for the faulty 404 handling [10/11/15]
        contentsync_path=data.localPath;
        window.localStorage.setItem("content_sync",JSON.stringify({"id":csync_id,"path":contentsync_path}));
        if(!fetch_libraries_completed) fetch_libraries_success()
    });

this is roughly what I'm doing, endsWith(data.localPath,".zip") checking .localPath, as it should be a folder instead of a local zip file that doesn't exist

imhotep commented 8 years ago

@kaansoral how are you calling ContentSync? Also don't store the path returned by ContentSync as it changes across app launches (use option type: 'local' instead).

kaansoral commented 8 years ago

var sync = ContentSync.sync({src:the_mobile_info.base_url+"/js/mobile.zip",id:'mlib'});

A bare call

I just checked why and what I'm caching, it seems I'm caching the path but not using it, I'm making a "local" call instead if there is cached content tho, so I'm using it as a flag to make a "local" call, whether it's needed or completely useless

Anyway, this issue occurs after the bare call tho, I just re-verified it

1) Call gets made for the .zip 2) Double instantenous 404's are logged (why double, I have no idea, with the retry, there are a total of 4 404's) 3) ContentSync reports success with a path to a non-existent zip file

I will make another clean build from scratch now, will report if there are changes

Edit: Re-tested a clean build, same issue, the conditional "local" call / localStorage caching is because the file url changes by the way, so I can't just call "local" all the time as I have to force a re-download when the url changes, this isn't related to the issue tho, I think the returned ".zip" path is the key to solve the issue, is there any valid scenario where the returned path is a ".zip"?

imhotep commented 8 years ago

@kaansoral seems like a bad rebase/merged occurred in latest version of this repository and caused some changes to disappear. Please try this.

cordova plugin add https://github.com/phonegap/phonegap-plugin-contentsync#9f7412d057dac8fdd01971be51c5a1ece2305b33

and get back to me.

kaansoral commented 8 years ago

Thanks, it works :) (No usage issues, There are some error logs, seems to be the aftermath of the previous issue, doesn't seem to affect the functionality)

I have to say, the support and development of this plugin is beyond awesome

While inspecting this issue I got to improve my plugin usage logic, so my effort wasn't wasted either

Also, the two http requests are HEAD + GET's, so that makes some sense too, wonder why the HEAD is for tho

CookieCookson commented 8 years ago

Had the same issue here, thanks @imhotep for the fix!

EDIT: My most favourite log so far... 'Deleting archive. It's probably an HTTP Error Page anyways'

imhotep commented 8 years ago

version 1.2.1 should fix the problem (I reapplied that commit on top of everything else).

CookieCookson commented 8 years ago

Can you make a release for 1.2.1?

imhotep commented 8 years ago

@CookieCookson but it is released: https://www.npmjs.com/package/phonegap-plugin-contentsync

CookieCookson commented 8 years ago

@imhotep Ah ok thanks, I was just referring to: https://github.com/phonegap/phonegap-plugin-contentsync/releases