Don't require loading of include.js on every pageview? #152
Comments
So right now, the way we do logout buttons is that we first run The problem with not including So what we really need is a way to logout and always call Alternatively, we can just ignore any auto-login and auto-logout stuff by refusing to respond to anything that wasn't initiated by In the meantime, perhaps you can use a script loader to load the Persona and django-browserid JS when the user clicks the button, and then trigger logout. |
I see, thank you for the explanation. Is there a reason django-browserid can't work that way by default? Inject |
If you decide to do it, you can as well switch to |
Hmm, but then my users won't be automatically logged in when they first sign up, will they? Still, it sounds enticing, thanks for the information. |
@wrr @skorokithakis I've asked someone else from the Identity team to hop in here and discuss the rationale behind our current design, but I'll try to touch on two of your main points in the meantime:
Would it be enough to put the script at the end of your page body or mark it deferred so that it doesn't block or slow down the rest of your site?
That sounds like something isn't configured quite right between your session management and the browserid API. Is there any way you could point me at your code so I can take a look and help out? |
I'm not sure, but I think Google counts the time until the full page load, so this would still count as "slow", plus, it's annoying to see the loading bar and makes the page feel slow.
Sure thing, it's http://staging.getinstabot.com. I will set up an account for you for your gmail email now, you should be able to just log in. If you browse around/refresh a few times, it'll happen. The site's sessions are fine, Persona just thinks the user has opted to log out and logs me out. I have third-party cookies disabled, if it helps diagnose. |
Can you reproduce the issue with third party cookies enabled, or with an exception added for |
Oh, I already had the exception added quite a while ago, turns out. This was happening with the exception in place, so it's not that issue. |
I've changed the logout onclick code to: e.preventDefault();
logoutRedirect = $(this).attr('href');
var head = document.getElementsByTagName('head')[0];
var script = document.createElement('script');
script.type= 'text/javascript';
script.src= 'https://login.persona.org/include.js';
head.appendChild(script);
script.onload = function () {
navigator.id.logout();
} I've verified that |
Persona has to do some setup before things like If you add an |
Hmm, I'm not sure how to even do that, unfortunately... This could be a good candidate for inclusion in the default
|
Alright. Brace yourselves, because I have done it, and it works, and I don't know whether it's brilliant or horrible (and when you don't know whether something you did is brilliant or horrible, it's not brilliant). I have tested the monster (if, admittedly, not thoroughly), on my site, and it behaves as expected. It loads the shim if it's not loaded, uses it if it is, logs out, logs in, and automatically logs in if the shim is loaded and the Persona session has not ended. As far as I can tell, it does the right thing on all circumstances, and removes the annoying logout bug (because it can't log out at its whim). Behold: /* This Source Code Form is subject to the terms of the Mozilla Public
* License, v. 2.0. If a copy of the MPL was not distributed with this
* file, You can obtain one at http://mozilla.org/MPL/2.0/. */
(function($) {
'use strict';
$(function() {
// State? Ewwwwww.
var loginRedirect = null; // Path to redirect to post-login.
var logoutRedirect = null; // Path to redirect to post-logout.
var $loginForm = $('#browserid-form'); // Form used to submit login.
var $browseridInfo = $('#browserid-info'); // Useful info from backend.
var requestArgs = $browseridInfo.data('requestArgs') || {};
var loginFailed = location.search.indexOf('bid_login_failed=1') !== -1;
var watchParams = {
loggedInUser: $browseridInfo.data('userEmail') || null,
onlogin: function(assertion) {
// Avoid auto-login on failure.
if (loginFailed) {
navigator.id.logout();
loginFailed = false;
return;
}
if (assertion) {
$loginForm.find('input[name="next"]').val(loginRedirect);
$loginForm.find('input[name="assertion"]').val(assertion);
$loginForm.submit();
}
},
onlogout: function() {
// Follow the logout link's href once logout is complete.
var currentLogoutUrl = logoutRedirect;
if (currentLogoutUrl !== null) {
logoutRedirect = null;
window.location = currentLogoutUrl;
} else {
// Sometimes you can get caught in a loop where BrowserID
// keeps trying to log you out as soon as watch is called,
// and fails since the logout URL hasn't been set yet.
// Here we just find the first logout button and use that
// URL; if this breaks your site, you'll just need custom
// JavaScript instead, sorry. :(
currentLogoutUrl = $('.browserid-logout').attr('href');
if (currentLogoutUrl) {
window.location = currentLogoutUrl;
}
}
}
}
// Call navigator.id.request whenever a login link is clicked.
$(document).on('click', '.browserid-login', function(e) {
e.preventDefault();
loginRedirect = $(this).data('next');
navigator.id.request(requestArgs);
});
// Call navigator.id.logout whenever a logout link is clicked.
$(document).on('click', '.browserid-logout', function(e) {
e.preventDefault();
logoutRedirect = $(this).attr('href');
if (!navigator.id) {
var head = document.getElementsByTagName('head')[0];
var script = document.createElement('script');
script.type= 'text/javascript';
script.src= 'https://login.persona.org/include.js';
head.appendChild(script);
script.onload = function() {
watchParams["onready"] = function() {navigator.id.logout()}
navigator.id.watch(watchParams)
}
} else {
navigator.id.logout()
}
});
if (navigator.id) navigator.id.watch(watchParams);
});
})(jQuery); |
w00t, awesome! (FYI I won't be able to look at this for a week or so, currently tied up at a work week) |
I'm the maintainer of the Drupal module: http://drupal.org/project/persona I find no front end issue with having the Persona JS shiv loading on every page. When JS aggregation is enabled, Drupal automatically downloads include.js and serves it directly from site aggregated with all the other JS that needs to be on every page. This avoids setting up a HTTPS connection to login.persona.org before the "load" event. I found that putting include.js at the bottom of the body rather than in head really slowed down page loads. There was a visible delay before Drupal's behaviours system kicked in. I also found a solution to the bug in Persona where it will issue a rouge onlogout if you navigate away from the page too soon:
Here is our JavaScript: http://drupalcode.org/project/persona.git/blob/HEAD:/persona.js |
That's very interesting, I'll try it, it might solve both my problems at once. Thank you very much for this. |
Is there any progress on this? I'm currently having problems with the JS again and it'd probably be nice if there were more eyes looking at this, by maybe integrating it to the mainline? |
@callahad Given that Persona is going to be moving away from managing sessions with it's API, what do you think of moving django-browserid to the As for solving this particular issue, we'd then use a script loader of some kind to load include.js from our own JavaScript. |
Don't move to .get -- it's not possible to make login work on Chrome/iOS, Windows Phone, or after email confirmation from the .get API. |
Looking back on this, I don't think adding built-in support for not loading the JavaScript on each page is a good idea. The new session-less API still requires pages to register callbacks via This does allow us to say "well, only include the JS on pages when the user is logged out" because, with the session-less API, there is no JavaScript to call for logging out. This is a valid point, but by that point it's likely the user has already cached the JS and won't be making more requests anyway. And it's trivial for sites using django-browserid to check for this and not include the JS themselves rather than build it into the library. I think adding an example of how to avoid including the JavaScript on logged-in pages to the documentation is the better fix. We still make it easy to figure out, but don't have to bother making our code more complex. Part of #201 involves adding a User Guide with examples of common usage patterns for django-browserid, and this would be a great fit. Thoughts? |
The problem I can see is that you have to load the JS if you want to log out. For sites that have a "log out" link on every page, you have to eventually load the JS on every page or you won't be able to log out... |
Here's a link to the PR with the new stateless API: mozilla/persona#3961 When using that new API (which hasn't landed in production yet but is oh-so-close), you don't have to call Because we switched to using ajax-based logins and logouts, without any changes you'll still need to load My suggestion is to include an example in the docs where you:
Those three steps should be all you need to have JS-less logouts. Given that they're not terribly hard and don't benefit much from being built-in to the library, I think documenting them instead of making it built-in somehow is the way to go. I hope that makes my previous comment a bit more clear. :D |
Ah, thanks for that, I wasn't aware of that API. This is how I was doing it initially, then you guys implemented stateful logins and I had to call Since I'm not familiar with the API, I assume it's similar to the first one, where you just need to include browserid.js in pages with "login" buttons, which sounds great to me. Thanks for your help! |
That's what we in the business like to call "listening to user feedback". :P
Yeah. If I'm reading that PR correctly, you just don't pass an |
Sounds great, thanks! |
Well, it took half a year, but the new stateless API has landed and there's a PR that has us switch over to it: #255 I believe that, as of that PR, you can safely only include |
I don't have any opinion one way or the other. |
Closing as an edge case. If it comes up as an issue again we'll reconsider. |
I would like to resurrect this. Persona makes seventeen requests on page load, even though the vast majority of my users will never log in. This is unacceptable to me, and I would like to dynamically inject include.js (or whatever is making the requests) after the user has clicked the login button. Is there a way to do this? I don't necessarily need this to be in |
I tried making a sample for this and failed as well, because every method of loading the JS in response to a user click I can think of or find functions asynchronously. This breaks because the popup box won't launch outside of a user-initiated event handler, and the async script loading breaks out of that event. Fixing this requires changes to Persona itself that I doubt will ever happen at this point. :( Seventeen requests on page load seems excessive, though. What is Persona doing with seventeen requests? |
It seems to be loading the images for the pop-up. That's odd. |
wat why why would it do that |
Maybe I implemented it wrong? See here: I'm just using django-browserid's form, though. |
I only see about 5 requests from Persona on page load. Or is this for when you're already logged in? |
No, this is on a brand-new incognito window: https://i.imgur.com/ASruDlR.png It's loading the iframe, so the iframe loads all this. The problem is that the site will have maybe one out of ten thousand page views logging in, so it's very rare that these need to be loaded :/ |
I still can't replicate your results. I don't get those resources being loaded. I can't think of a reason why you're seeing them and I'm not. |
Hmm, neither can this: http://www.webpagetest.org/result/151217_2S_13ZJ/1/details Must be something in my setup, sorry about that. |
Is there a way to not require
include.js
to be loaded on every pageview? I was pretty happy with BrowserID when that script only needed to be loaded on my landing page (for logins), but now it needs to load on every single page load, slowing down my page, even though a very very small percentage of pageviews lead to a log out.It's also causing problems by apparently thinking that my session is expired and forces a logout, only to be led to the front page and automatically log in again. As you can imagine, this is a dealbreaker for users.
Just not loading it doesn't work, because clicking the "logout" button leaves users on the landing page, where BrowserID promptly logs back in again.
Is there a way to prevent loading the script until the user actually clicks the logout button, or similar?
The text was updated successfully, but these errors were encountered: