Fallback for keepalive js #16452

andrepereiradasilva · 2017-06-02T17:50:17Z

Pull Request for Issue #16375 (only keepalive fallback part).

Summary of Changes

The keepalive js fallback is not working correctly when there is no system.keepalive json. This PR intends to add better fallback for those wild scenarios.

Testing Instructions

Pre-requisites:

Use latest joomla staging with a login module in the frontend (so keepalive is called)
In global config set joomla session to 1 minute (for faster test) and System Cache to "ON - Progressive caching"

Reproducing the issue:

Clear joomla cache
Go to frontend - not logged, reload the page two times (Ctrl+F5)
Confirm there is a js error in the Chrome Debug Console "Console" tab (F12)

Test if proposed keepalive fallback works:

Apply patch Fix initial call of Joomla.loadOptions() #16488 (if not merged) and them this patch
Repeat steps in "Reproducing the issue", but now confirm there is no js error in the Chrome Debug Console "Console" tab (F12)
Wait one minute and also confirm keepalive in called correctly in Chrome Debug Console "Network" tab

Expected result

Keepalive should always work even if there is no system.keepalive json.

Actual result

Keepalive does not work in this scenario because of a js error.

Documentation Changes Required

@dgt41 @Fedik can you also check this one and confirm there is no problem in adding the joomla install uri paths trough script options in behavior.core, ie, all the pages that use the core bahaviour (this would also be used in #15529).

…ny js can use it

Fedik · 2017-06-03T12:13:10Z

@andrepereiradasilva I did not tested, but looks good to me

Fedik · 2017-06-03T12:16:43Z

media/system/js/keepalive-uncompressed.js

+		    keepaliveInterval = keepaliveOptions && keepaliveOptions.interval ? keepaliveOptions.interval : 45 * 1000;
+
+		// Fallback in case no keepalive uri was found.
+		if (keepaliveUri === '')


this check only for empty string, for check all "false" cases use if (!keepaliveUri)

yes but i want to strict check for empty string :) see above

keepaliveUri = keepaliveOptions && keepaliveOptions.uri ? keepaliveOptions.uri.replace(/&/g, '&') : '',

dgrammatiko · 2017-06-03T14:54:23Z

@Fedik can you also update the spinning logo to move this code instead of the data-attr (#15529)?

Quy · 2017-06-03T15:19:55Z

libraries/cms/html/behavior.php

@@ -88,6 +88,10 @@ public static function core()
 		}

 		JHtml::_('script', 'system/core.js', array('version' => 'auto', 'relative' => true));
+
+		// Add core paths so javascript can have the root path.
+		JFactory::getDocument()->addScriptOptions('system.paths', array('root' => JUri::root(true), 'base', JUri::base(true)));


Change to 'base' => JUri::base(true)?

Quy · 2017-06-03T15:21:26Z

After PR, I still get a js error:

Uncaught TypeError: Cannot read property 'system.keepalive' of null
    at Object.e.getOptions (core.js?449d852…:1)
    at HTMLDocument.<anonymous> (keepalive.js?449d852…:1)

Fedik · 2017-06-03T15:53:43Z

@andrepereiradasilva I guess there still bug with Joomla.loadOptions

there

if (!Joomla.optionsStorage) {
  Joomla.optionsStorage = options;
}

need to be:

if (!Joomla.optionsStorage) {
  Joomla.optionsStorage = options || {};
}

Can you add it here or I do new PR?

@dgt41 I thought that is your PR 😉
we can update, after this one will be merged

Fedik · 2017-06-03T16:07:03Z

another strange thing,
with this patch, if cache is enabled, the core.js loaded but there no any <script type="application/json" class="joomla-script-options"> in the head.
maybe it somehow related to JCache::set(get)Workarounds ?

upd. debug off, and only on "Progressive caching"

andrepereiradasilva · 2017-06-03T16:15:58Z

@Fedik

Can you add it here or I do new PR?

please do a PR i prefer you do that Joomla.optionsStorage part - i will test it so it can be RTC and merged fast (hope @dgt41 tests it too 😄 )
this one stays in standby until your is merged

another strange thing,
with this patch, if cache is enabled, the core.js loaded but there no any <script type="application/json" class="joomla-script-options"> in the head.
maybe it somehow related to JCache::set(get)Workarounds ?

that is the first problem of the issue that originated this PR see #16375 (comment)
But that's another issue not related to js.

Fedik · 2017-06-03T16:36:21Z

@andrepereiradasilva there is pull #16488 😉
which should fix:

Uncaught TypeError: Cannot read property 'system.keepalive' of null
    at Object.e.getOptions (core.js?449d852…:1)
    at HTMLDocument.<anonymous> (keepalive.js?449d852…:1)

Current pull should work after #16488

andrepereiradasilva · 2017-06-03T16:42:24Z

yeah this work after #16488 applied

Quy · 2017-06-03T17:10:53Z

I am still getting the js error after both PRs are applied in the frontend.

Uncaught TypeError: Cannot read property 'system.keepalive' of null
    at Object.Joomla.getOptions (core.js?cf50a49…:1)
    at HTMLDocument.<anonymous> (keepalive.js?cf50a49…:1)

andrepereiradasilva · 2017-06-03T17:14:45Z

@Quy did you refresh the js cache?

UPDATE: Forget it i'm seing it also

andrepereiradasilva · 2017-06-03T18:07:30Z

@Quy : @Fedik solved the issue in the other PR, should be fine now

Quy · 2017-06-03T18:08:41Z

I have tested this item ✅ successfully on 80d499b

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/16452.}

Fedik · 2017-06-03T18:10:50Z

I have tested this item ✅ successfully on 80d499b

works as described, with #16488

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/16452.}

ghost · 2017-06-03T18:12:00Z

RTC after two successful tests.

andrepereiradasilva added 5 commits June 2, 2017 18:20

add better fallback to keepalive

08f3e2f

add minified script

1fc04f9

add Joomla instalation url core and base paths to core behaviour so a…

5fee537

…ny js can use it

systemPaths, not systemPath

627391a

better comment

6913407

joomla-cms-bot added the PR-staging label Jun 2, 2017

andrepereiradasilva mentioned this pull request Jun 2, 2017

behavior.keepalive with progressive cache JS error #16375

Closed

Merge branch 'staging' into patch-20

ed4a177

Fedik reviewed Jun 3, 2017

View reviewed changes

Quy reviewed Jun 3, 2017

View reviewed changes

ups ... sorry

80d499b

andrepereiradasilva mentioned this pull request Jun 3, 2017

Fix initial call of Joomla.loadOptions() #16488

Merged

joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Jun 3, 2017

rdeutz merged commit 9c36589 into joomla:staging Jun 13, 2017

joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Jun 13, 2017

rdeutz added this to the Joomla 3.7.3 milestone Jun 13, 2017

andrepereiradasilva deleted the patch-20 branch June 13, 2017 14:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fallback for keepalive js #16452

Fallback for keepalive js #16452

andrepereiradasilva commented Jun 2, 2017 •

edited by joomla-cms-bot

Fedik commented Jun 3, 2017

Fedik Jun 3, 2017

andrepereiradasilva Jun 3, 2017

Fedik Jun 3, 2017

dgrammatiko commented Jun 3, 2017

Quy Jun 3, 2017

Quy commented Jun 3, 2017

Fedik commented Jun 3, 2017

Fedik commented Jun 3, 2017 •

edited

andrepereiradasilva commented Jun 3, 2017 •

edited

Fedik commented Jun 3, 2017 •

edited

andrepereiradasilva commented Jun 3, 2017

Quy commented Jun 3, 2017 •

edited

andrepereiradasilva commented Jun 3, 2017 •

edited

andrepereiradasilva commented Jun 3, 2017

Quy commented Jun 3, 2017

Fedik commented Jun 3, 2017

ghost commented Jun 3, 2017

Fallback for keepalive js #16452

Fallback for keepalive js #16452

Conversation

andrepereiradasilva commented Jun 2, 2017 • edited by joomla-cms-bot

Summary of Changes

Testing Instructions

Expected result

Actual result

Documentation Changes Required

Fedik commented Jun 3, 2017

Fedik Jun 3, 2017

Choose a reason for hiding this comment

andrepereiradasilva Jun 3, 2017

Choose a reason for hiding this comment

Fedik Jun 3, 2017

Choose a reason for hiding this comment

dgrammatiko commented Jun 3, 2017

Quy Jun 3, 2017

Choose a reason for hiding this comment

Quy commented Jun 3, 2017

Fedik commented Jun 3, 2017

Fedik commented Jun 3, 2017 • edited

andrepereiradasilva commented Jun 3, 2017 • edited

Fedik commented Jun 3, 2017 • edited

andrepereiradasilva commented Jun 3, 2017

Quy commented Jun 3, 2017 • edited

andrepereiradasilva commented Jun 3, 2017 • edited

andrepereiradasilva commented Jun 3, 2017

Quy commented Jun 3, 2017

Fedik commented Jun 3, 2017

ghost commented Jun 3, 2017

andrepereiradasilva commented Jun 2, 2017 •

edited by joomla-cms-bot

Fedik commented Jun 3, 2017 •

edited

andrepereiradasilva commented Jun 3, 2017 •

edited

Fedik commented Jun 3, 2017 •

edited

Quy commented Jun 3, 2017 •

edited

andrepereiradasilva commented Jun 3, 2017 •

edited