Skip to content
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

Reload on Resume doesn't work #10

Closed
tafelito opened this issue Sep 28, 2016 · 23 comments
Closed

Reload on Resume doesn't work #10

tafelito opened this issue Sep 28, 2016 · 23 comments

Comments

@tafelito
Copy link

Hi guys, I was trying to use this package for a production ready app I've been working and I can get this package to work. I debugged it and I realize that it looks like there is an error, for e.g when you do use this within the Tracker or a SetTimeout, beacuse this is a reference to the thread running inside the Tracker. Am I missing something here? Because this.updateAvailable() its always undefinded

Tracker.autorun((computation) => {
        if (this.updateAvailable()) {
          this.reload();
        }

        this._waitForUpdate(computation);
      });

Thank you!

@lorensr
Copy link
Collaborator

lorensr commented Sep 28, 2016

Hi, your code looks a little different than master – are you on v1.2.1?

https://github.com/jamielob/reloader/blob/master/reloader.js#L98-L112

@tafelito
Copy link
Author

tafelito commented Sep 28, 2016

Sorry, I copied the one from v2, I'm actually using v1.2.1, from the master and the code look like this

 Tracker.autorun((c) => {

        if (this.updateAvailable.get()) {
          this.reload();
        }

        this._waitForUpdate(c)

      });

@tafelito
Copy link
Author

tafelito commented Sep 28, 2016

Not sure if that's the error, but what's happening now is that I'm running my app, I update some code, I put the app into the background, wait 30 sec ( I set the idleCutoff time to 30 sec just for testing purposes) and when I resume the app, it never hides the splash screen. So I started debugging it, and that's when I saw that as the possible error

@lorensr
Copy link
Collaborator

lorensr commented Sep 28, 2016

the () => binds this, so that shouldn't be the issue.

  1. I'm running my app,
  2. I update some code,
  3. I put the app into the background,
  4. wait 30 sec ( I set the idleCutoff time to 30 sec just for testing purposes) and when
  5. I resume the app, it never hides the splash screen

after step 2, do you wait for the update to download before doing 3, or do 3 at the same time as 2?

what's your Reloader.configure call?

@tafelito
Copy link
Author

tafelito commented Sep 28, 2016

When I debug it, this.updateAvailable is undefined. I tried both, updating the app while running, wait till get the updates, and go to the background, and going to the background, updating, and then resume. This is my configuration

if(Meteor.isClient) {
  Reloader.configure({
    check: 'everyStart', // Check for new code every time the app starts 
    checkTimer: 3000,  // Wait 3 seconds to see if new code is available
    refresh: 'startAndResume', // Refresh to already downloaded code on both start and resume
    idleCutoff: 1000 * 30 * 1  // Wait 10 minutes before treating a resume as a start
  });
}

@lorensr
Copy link
Collaborator

lorensr commented Sep 28, 2016

Ok, with that configuration:

After 2, wait until Reloader.updateAvailable.get() in the browser console is true. Then put in background and resume (don't need to wait, it just needs the document 'resume' event). Now it should refresh to latest version, and show launch screen during refresh, and Reloader.updateAvailable.get() should be false.

If that doesn't happen, not sure why. You can try debugging starting at

https://github.com/jamielob/reloader/blob/master/reloader.js#L195

and making sure it gets to this line:

https://github.com/jamielob/reloader/blob/master/reloader.js#L150

@tafelito
Copy link
Author

tafelito commented Sep 28, 2016

Ok I did that and it still does the same thing, it shoes the splash screen, and never goes away

This is being called

https://github.com/jamielob/reloader/blob/master/reloader.js#L195

but then here, shouldCheck is always true, so it shows the splash screen and then it calls _waitForUpdate. Then the reload is called, and when it calls this line window.location.replace(window.location.href); it does nothing. I'm using iOS 10, not sure if that could be the error also. But after that, splash screen stays there

@tafelito
Copy link
Author

I'm also getting this error message sometimes

Error: Skipping downloading blacklisted version

@tafelito
Copy link
Author

Ok I managed to get this line called

https://github.com/jamielob/reloader/blob/master/reloader.js#L150

But still the same issue, when the reload is called, it calls the preload, shows the splash, and then it does window.location.replace(window.location.href); After that, nothing happen

Any ideas?

@lorensr
Copy link
Collaborator

lorensr commented Sep 28, 2016

Skipping blacklisted error usually happens when there's a JS error in the Safari remote console.

Assuming no JS errors, then the window.location.replace(window.location.href) should reload the page behind the splashscreen (you should notice it reloading in safari console), and then this is called:

https://github.com/jamielob/reloader/blob/master/reloader.js#L114

Which should at some point call launchscreen.release()

@tafelito
Copy link
Author

Ok so that's the issue then, after window.location.replace(window.location.href) the _onPageLoad() is never called, not sure why

@lorensr
Copy link
Collaborator

lorensr commented Sep 28, 2016

It happens when the JS file is run, which should be each page load.

https://github.com/jamielob/reloader/blob/master/reloader.js#L188

Is the page definitely reloading?

@tafelito
Copy link
Author

ok I found why, it might be related to this issue

meteor/cordova-plugin-meteor-webapp#12

If I use this instead, it does work

const urlSansPath = `${window.location.protocol}//${window.location.host}/`;
window.location.replace(urlSansPath);

@tafelito
Copy link
Author

tafelito commented Sep 28, 2016

Doing window.location.reload() also works

I might have to fork the repo and replace that, or do you think we can include that fix on the main repo?

@lorensr
Copy link
Collaborator

lorensr commented Sep 28, 2016

It looked from the issue that it was only in cases in which that non-root path had an error? For people who don't have this issue, I'd like to keep this working: user on /post and when they resume, the reload puts them back on /post.

We're using replace because of this: https://github.com/meteor/cordova-plugin-meteor-webapp#switching-to-a-real-embedded-web-server-ios-only

@tafelito
Copy link
Author

Doing window.location.reload() wouldn't take the user to the same path they are before reloading?

When I inspect the window.location.href before doing the replace, this is what it has

http://localhost:12024/?cdvToken=E462B92A-4B1E-4988-A598-BF9438537A14-454-0000002EC83F9D69#!home"

I don't see any error there

@lorensr
Copy link
Collaborator

lorensr commented Sep 29, 2016

Sorry, this was relevant to window.location.replace(urlSansPath); :

It looked from the issue that it was only in cases in which that non-root path had an error? For people who don't have this issue, I'd like to keep this working: user on /post and when they resume, the reload puts them back on /post.

And this was relevant to .reload():

We're using replace because of this: https://github.com/meteor/cordova-plugin-meteor-webapp#switching-to-a-real-embedded-web-server-ios-only

@tafelito
Copy link
Author

So of what I've been testing so far, it looks like the replace doesn't like any url with parameters. So because I have a cvdToken on my url, the replace won't do anything. So if I instead of doing

window.location.replace("http://localhost:12024/?cdvToken=E462B92A-4B1E-4988-A598-BF9438537A14-454-0000002EC83F9D69#!home");

I do

window.location.replace("http://localhost:12024/#!home");

That works, but of course, the user lose the authentication and its logged out

Did you have any errors before, with url paths that has parameters?

@tafelito
Copy link
Author

Ok one more thing, what I just noticed is that the replace is actually working, it does switch the location, but it does not fire the onPageLoad event, because it does not reload the page.

If I replace the url with a different route, the location it does change, but does not reload. I'm using Iron Router for the routing, could that be something?

@lorensr
Copy link
Collaborator

lorensr commented Sep 29, 2016

Did you have any errors before, with url paths that has parameters?

I haven't, no.

Ok one more thing, what I just noticed is that the replace is actually working, it does switch the location, but it does not fire the onPageLoad event, because it does not reload the page.

Looks like location.replace doesn't reload if you're only changing the hash (after #). IR supports history API (not using #!), and most people do that.

@tafelito
Copy link
Author

You mean replacing the window.location.replace by using History.go()?

@tafelito
Copy link
Author

tafelito commented Oct 3, 2016

Hi @lorensr, I was checking at the Reload.js and I see that they also check for the # at the end of the url to do a replace or a reload, otherwise it doesn't work. Have you seen this?

[https://github.com/meteor/meteor/blob/devel/packages/reload/reload.js#L219]

@lorensr lorensr closed this as completed in 064c1fb Oct 6, 2016
@lorensr
Copy link
Collaborator

lorensr commented Oct 6, 2016

Added that, thanks tafelito 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants