You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@wicket.apache.org by svenmeier <gi...@git.apache.org> on 2018/01/02 10:28:18 UTC
[GitHub] wicket pull request #253: WICKET-6498 deferred javascript
GitHub user svenmeier opened a pull request:
https://github.com/apache/wicket/pull/253
WICKET-6498 deferred javascript
If addEventListener is used instead of jQuery "domready" and "load" events, and OnLoadHeaderItem instead of JavaScriptHeaderItem, all JavaScript resources can be deferred.
Open questions:
- does this solution work reliably on all browsers?
- who decides which JavaScript resources to defer and which not?
- how to handle cases where user code is still using a JavaScriptHeaderItem?
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/apache/wicket WICKET-6498_deferred_javascript
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/wicket/pull/253.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #253
----
commit d77ba274f38953b6d5ab1088c7a9555111acd4fc
Author: Sven Meier <sv...@...>
Date: 2018-01-02T10:20:03Z
WICKET-6498 deferred javascript
use OnLoadHeaderItem instead of JavaScriptHeaderItem, addEventListener instead of jQuery;
defer all JavaScript resources for now
----
---
[GitHub] wicket pull request #253: WICKET-6498 deferred javascript
Posted by klopfdreh <gi...@git.apache.org>.
Github user klopfdreh commented on a diff in the pull request:
https://github.com/apache/wicket/pull/253#discussion_r159220379
--- Diff: wicket-core/src/main/java/org/apache/wicket/resource/CoreLibrariesContributor.java ---
@@ -63,8 +64,7 @@ public static void contributeAjax(final Application application, final IHeaderRe
if (debugSettings.isAjaxDebugModeEnabled())
{
response.render(JavaScriptHeaderItem.forReference(jsLibrarySettings.getWicketAjaxDebugReference()));
- response.render(JavaScriptHeaderItem.forScript("Wicket.Ajax.DebugWindow.enabled=true;",
- "wicket-ajax-debug-enable"));
--- End diff --
Same as wicket-ajax-base-url
---
[GitHub] wicket pull request #253: WICKET-6498 deferred javascript
Posted by klopfdreh <gi...@git.apache.org>.
Github user klopfdreh commented on a diff in the pull request:
https://github.com/apache/wicket/pull/253#discussion_r159219467
--- Diff: wicket-core/src/main/java/org/apache/wicket/markup/head/OnLoadHeaderItem.java ---
@@ -86,8 +86,8 @@ public void render(Response response)
CharSequence js = getJavaScript();
if (Strings.isEmpty(js) == false)
{
- JavaScriptUtils.writeJavaScript(response, "Wicket.Event.add(window, \"load\", " +
- "function(event) { " + js + ";});");
+ JavaScriptUtils.writeJavaScript(response,
+ "var f = function() {" + js + ";};\nif ('loaded' === document.readyState) f(); else window.addEventListener('load', f);");
--- End diff --
Is loaded the correct state for sure?
https://developer.mozilla.org/de/docs/Web/API/Document/readyState
here "complete" is mentioned. I also remember that there were some other pitfalls with the readyState for some browsers.
This german site mentions that "loaded" is only available in IE.
https://wiki.selfhtml.org/wiki/JavaScript/DOM/Document/readyState
---
[GitHub] wicket pull request #253: WICKET-6498 deferred javascript
Posted by solomax <gi...@git.apache.org>.
Github user solomax commented on a diff in the pull request:
https://github.com/apache/wicket/pull/253#discussion_r161151287
--- Diff: wicket-core/src/main/java/org/apache/wicket/ajax/AbstractDefaultAjaxBehavior.java ---
@@ -106,8 +107,7 @@ public void renderHead(final Component component, final IHeaderResponse response
RequestCycle requestCycle = component.getRequestCycle();
Url baseUrl = requestCycle.getUrlRenderer().getBaseUrl();
CharSequence ajaxBaseUrl = Strings.escapeMarkup(baseUrl.toString());
- response.render(JavaScriptHeaderItem.forScript("Wicket.Ajax.baseUrl=\"" + ajaxBaseUrl
- + "\";", "wicket-ajax-base-url"));
--- End diff --
previous code set the id attribute, so the check was trivial (node.attr === 'wicket-ajax-base-url'). Current implementation adds text content. which have to be parsed, maybe it worth to set attribute back to avoid parsing the text?
---
[GitHub] wicket pull request #253: WICKET-6498 deferred javascript
Posted by svenmeier <gi...@git.apache.org>.
Github user svenmeier commented on a diff in the pull request:
https://github.com/apache/wicket/pull/253#discussion_r162726991
--- Diff: wicket-core/src/main/java/org/apache/wicket/markup/head/filter/FilteringHeaderResponse.java ---
@@ -263,14 +262,12 @@ public void markRendered(Object object)
}
};
- ResourceAggregator resourceAggregator = new ResourceAggregator(headerRenderer);
-
for (HeaderItem curItem : resp)
{
- resourceAggregator.render(curItem);
+ headerRenderer.render(curItem);
--- End diff --
Thanks Maxim, I'll take a look.
---
[GitHub] wicket pull request #253: WICKET-6498 deferred javascript
Posted by svenmeier <gi...@git.apache.org>.
Github user svenmeier commented on a diff in the pull request:
https://github.com/apache/wicket/pull/253#discussion_r161204652
--- Diff: wicket-core/src/main/java/org/apache/wicket/ajax/AbstractDefaultAjaxBehavior.java ---
@@ -106,8 +107,7 @@ public void renderHead(final Component component, final IHeaderResponse response
RequestCycle requestCycle = component.getRequestCycle();
Url baseUrl = requestCycle.getUrlRenderer().getBaseUrl();
CharSequence ajaxBaseUrl = Strings.escapeMarkup(baseUrl.toString());
- response.render(JavaScriptHeaderItem.forScript("Wicket.Ajax.baseUrl=\"" + ajaxBaseUrl
- + "\";", "wicket-ajax-base-url"));
--- End diff --
Setting the baseUrl has to be deferred until onLoad, otherwise the deferred wicket-ajax-jquery.js hasn't been loaded yet and Wicket.Ajax is not defined.
---
[GitHub] wicket issue #253: WICKET-6498 deferred javascript
Posted by svenmeier <gi...@git.apache.org>.
Github user svenmeier commented on the issue:
https://github.com/apache/wicket/pull/253
Many thanks Maxim for your work on this!
But with #258 we have a much simpler solution now, that should please those that are sceptical about the changes presented here.
If no one objects, I'll close this pull request.
---
[GitHub] wicket issue #253: WICKET-6498 deferred javascript
Posted by solomax <gi...@git.apache.org>.
Github user solomax commented on the issue:
https://github.com/apache/wicket/pull/253
Sure :) no rush, just would like to check if everyone happy with this PR :)
---
[GitHub] wicket issue #253: WICKET-6498 deferred javascript
Posted by svenmeier <gi...@git.apache.org>.
Github user svenmeier commented on the issue:
https://github.com/apache/wicket/pull/253
Nope, not yet please. I have a new idea I want to try out.
---
[GitHub] wicket pull request #253: WICKET-6498 deferred javascript
Posted by svenmeier <gi...@git.apache.org>.
Github user svenmeier commented on a diff in the pull request:
https://github.com/apache/wicket/pull/253#discussion_r161203809
--- Diff: wicket-core/src/main/java/org/apache/wicket/markup/head/OnDomReadyHeaderItem.java ---
@@ -86,8 +86,8 @@ public void render(Response response)
CharSequence js = getJavaScript();
if (Strings.isEmpty(js) == false)
{
- JavaScriptUtils.writeJavaScript(response, "Wicket.Event.add(window, \"domready\", " +
- "function(event) { " + js + ";});");
+ JavaScriptUtils.writeJavaScript(response,
+ "var f = function() {" + js + ";};\nif ('loading' !== document.readyState) f(); else document.addEventListener('DOMContentLoaded', f);");
--- End diff --
That does no harm +1
---
[GitHub] wicket pull request #253: WICKET-6498 deferred javascript
Posted by svenmeier <gi...@git.apache.org>.
Github user svenmeier commented on a diff in the pull request:
https://github.com/apache/wicket/pull/253#discussion_r159297861
--- Diff: wicket-core/src/main/java/org/apache/wicket/markup/head/OnLoadHeaderItem.java ---
@@ -86,8 +86,8 @@ public void render(Response response)
CharSequence js = getJavaScript();
if (Strings.isEmpty(js) == false)
{
- JavaScriptUtils.writeJavaScript(response, "Wicket.Event.add(window, \"load\", " +
- "function(event) { " + js + ";});");
+ JavaScriptUtils.writeJavaScript(response,
+ "var f = function() {" + js + ";};\nif ('loaded' === document.readyState) f(); else window.addEventListener('load', f);");
--- End diff --
You're right, "complete" it should be.
---
[GitHub] wicket issue #253: WICKET-6498 deferred javascript
Posted by svenmeier <gi...@git.apache.org>.
Github user svenmeier commented on the issue:
https://github.com/apache/wicket/pull/253
@solomax I'm sorry for all the hard work you put into fixing the tests, but #258 is much better than this initial solution. Closing.
---
[GitHub] wicket pull request #253: WICKET-6498 deferred javascript
Posted by solomax <gi...@git.apache.org>.
Github user solomax commented on a diff in the pull request:
https://github.com/apache/wicket/pull/253#discussion_r161372957
--- Diff: wicket-core/src/main/java/org/apache/wicket/markup/head/filter/FilteringHeaderResponse.java ---
@@ -263,14 +262,12 @@ public void markRendered(Object object)
}
};
- ResourceAggregator resourceAggregator = new ResourceAggregator(headerRenderer);
-
for (HeaderItem curItem : resp)
{
- resourceAggregator.render(curItem);
+ headerRenderer.render(curItem);
--- End diff --
Unfortunately with this change `FilteringHeaderResponseTest` fails due to JS dependencies are not being rendered :(((
@svenmeier Could you please take a look at this?
---
[GitHub] wicket issue #253: WICKET-6498 deferred javascript
Posted by solomax <gi...@git.apache.org>.
Github user solomax commented on the issue:
https://github.com/apache/wicket/pull/253
Thanks a lot @svenmeier!
All tests are green now :)
Shall we merge?
---
[GitHub] wicket pull request #253: WICKET-6498 deferred javascript
Posted by klopfdreh <gi...@git.apache.org>.
Github user klopfdreh commented on a diff in the pull request:
https://github.com/apache/wicket/pull/253#discussion_r159220270
--- Diff: wicket-core/src/main/java/org/apache/wicket/ajax/AbstractDefaultAjaxBehavior.java ---
@@ -106,8 +107,7 @@ public void renderHead(final Component component, final IHeaderResponse response
RequestCycle requestCycle = component.getRequestCycle();
Url baseUrl = requestCycle.getUrlRenderer().getBaseUrl();
CharSequence ajaxBaseUrl = Strings.escapeMarkup(baseUrl.toString());
- response.render(JavaScriptHeaderItem.forScript("Wicket.Ajax.baseUrl=\"" + ajaxBaseUrl
- + "\";", "wicket-ajax-base-url"));
--- End diff --
Mh I didn't look into the implementation but is wicket-ajax-base-url not used to be applied to the OnLoadHeaderItem, too. I could imagine that the information gets lost in this case.
---
[GitHub] wicket issue #253: WICKET-6498 deferred javascript
Posted by solomax <gi...@git.apache.org>.
Github user solomax commented on the issue:
https://github.com/apache/wicket/pull/253
NP, I'll check the tests again :)
---
[GitHub] wicket pull request #253: WICKET-6498 deferred javascript
Posted by svenmeier <gi...@git.apache.org>.
Github user svenmeier closed the pull request at:
https://github.com/apache/wicket/pull/253
---
[GitHub] wicket pull request #253: WICKET-6498 deferred javascript
Posted by solomax <gi...@git.apache.org>.
Github user solomax commented on a diff in the pull request:
https://github.com/apache/wicket/pull/253#discussion_r161151470
--- Diff: wicket-core/src/main/java/org/apache/wicket/markup/head/OnDomReadyHeaderItem.java ---
@@ -86,8 +86,8 @@ public void render(Response response)
CharSequence js = getJavaScript();
if (Strings.isEmpty(js) == false)
{
- JavaScriptUtils.writeJavaScript(response, "Wicket.Event.add(window, \"domready\", " +
- "function(event) { " + js + ";});");
+ JavaScriptUtils.writeJavaScript(response,
+ "var f = function() {" + js + ";};\nif ('loading' !== document.readyState) f(); else document.addEventListener('DOMContentLoaded', f);");
--- End diff --
I would like to change this code to be:
```
{ //scope
const f = function() {" + js + ";}; ....
}
```
This way multiple `OnDomReadyHeader` items will work as expected in deferred mode
WDYT?
---
[GitHub] wicket pull request #253: WICKET-6498 deferred javascript
Posted by klopfdreh <gi...@git.apache.org>.
Github user klopfdreh commented on a diff in the pull request:
https://github.com/apache/wicket/pull/253#discussion_r159287015
--- Diff: wicket-core/src/main/java/org/apache/wicket/markup/head/OnLoadHeaderItem.java ---
@@ -86,8 +86,8 @@ public void render(Response response)
CharSequence js = getJavaScript();
if (Strings.isEmpty(js) == false)
{
- JavaScriptUtils.writeJavaScript(response, "Wicket.Event.add(window, \"load\", " +
- "function(event) { " + js + ";});");
+ JavaScriptUtils.writeJavaScript(response,
+ "var f = function() {" + js + ";};\nif ('loaded' === document.readyState) f(); else window.addEventListener('load', f);");
--- End diff --
Here I suggest to change it to „complete“
---
Re: [GitHub] wicket pull request #253: WICKET-6498 deferred javascript
Posted by Tobias Soloschenko <to...@googlemail.com>.
Hi,
I just reviewed it and added some notes.
kind regards
Tobias
> Am 02.01.2018 um 11:28 schrieb svenmeier <gi...@git.apache.org>:
>
> GitHub user svenmeier opened a pull request:
>
> https://github.com/apache/wicket/pull/253
>
> WICKET-6498 deferred javascript
>
> If addEventListener is used instead of jQuery "domready" and "load" events, and OnLoadHeaderItem instead of JavaScriptHeaderItem, all JavaScript resources can be deferred.
>
> Open questions:
> - does this solution work reliably on all browsers?
> - who decides which JavaScript resources to defer and which not?
> - how to handle cases where user code is still using a JavaScriptHeaderItem?
>
> You can merge this pull request into a Git repository by running:
>
> $ git pull https://github.com/apache/wicket WICKET-6498_deferred_javascript
>
> Alternatively you can review and apply these changes as the patch at:
>
> https://github.com/apache/wicket/pull/253.patch
>
> To close this pull request, make a commit to your master/trunk branch
> with (at least) the following in the commit message:
>
> This closes #253
>
> ----
> commit d77ba274f38953b6d5ab1088c7a9555111acd4fc
> Author: Sven Meier <sv...@...>
> Date: 2018-01-02T10:20:03Z
>
> WICKET-6498 deferred javascript
>
> use OnLoadHeaderItem instead of JavaScriptHeaderItem, addEventListener instead of jQuery;
> defer all JavaScript resources for now
>
> ----
>
>
> ---
[GitHub] wicket pull request #253: WICKET-6498 deferred javascript
Posted by svenmeier <gi...@git.apache.org>.
Github user svenmeier commented on a diff in the pull request:
https://github.com/apache/wicket/pull/253#discussion_r161314468
--- Diff: wicket-core/src/main/java/org/apache/wicket/markup/head/ResourceAggregator.java ---
@@ -337,24 +338,27 @@ private void renderCombinedEventScripts()
if (combinedScript.length() > 0)
{
combinedScript.append("\nWicket.Event.publish(Wicket.Event.Topic.AJAX_HANDLERS_BOUND);");
- getRealResponse().render(
- OnDomReadyHeaderItem.forScript(combinedScript.append('\n').toString()));
+ getRealResponse().render(OnDomReadyHeaderItem.forScript(combinedScript.append('\n')));
}
combinedScript.setLength(0);
+ String lastId = null;
--- End diff --
That doesn't seem to be reliable: what if multiple items are given an id?
IMHO we shouldn't changed code just to please test code. Couldn't BaseWicketTester just parse the url from the whole JavaScript via a regular expression?
> `.*Wicket\.Ajax\.baseUrl="(.*)";.*`
---
[GitHub] wicket pull request #253: WICKET-6498 deferred javascript
Posted by klopfdreh <gi...@git.apache.org>.
Github user klopfdreh commented on a diff in the pull request:
https://github.com/apache/wicket/pull/253#discussion_r159286930
--- Diff: wicket-core/src/main/java/org/apache/wicket/resource/CoreLibrariesContributor.java ---
@@ -63,8 +64,7 @@ public static void contributeAjax(final Application application, final IHeaderRe
if (debugSettings.isAjaxDebugModeEnabled())
{
response.render(JavaScriptHeaderItem.forReference(jsLibrarySettings.getWicketAjaxDebugReference()));
- response.render(JavaScriptHeaderItem.forScript("Wicket.Ajax.DebugWindow.enabled=true;",
- "wicket-ajax-debug-enable"));
--- End diff --
Also ignore this. :-)
---
[GitHub] wicket pull request #253: WICKET-6498 deferred javascript
Posted by svenmeier <gi...@git.apache.org>.
Github user svenmeier commented on a diff in the pull request:
https://github.com/apache/wicket/pull/253#discussion_r161148721
--- Diff: wicket-core/src/main/java/org/apache/wicket/ajax/AbstractDefaultAjaxBehavior.java ---
@@ -106,8 +107,7 @@ public void renderHead(final Component component, final IHeaderResponse response
RequestCycle requestCycle = component.getRequestCycle();
Url baseUrl = requestCycle.getUrlRenderer().getBaseUrl();
CharSequence ajaxBaseUrl = Strings.escapeMarkup(baseUrl.toString());
- response.render(JavaScriptHeaderItem.forScript("Wicket.Ajax.baseUrl=\"" + ajaxBaseUrl
- + "\";", "wicket-ajax-base-url"));
--- End diff --
It's not missing, it's just on a different location (see below), where getWicketAjaxBaseUrlEncodedInLastResponse() doesn't find it - we have to change that method accordingly.
---
[GitHub] wicket pull request #253: WICKET-6498 deferred javascript
Posted by klopfdreh <gi...@git.apache.org>.
Github user klopfdreh commented on a diff in the pull request:
https://github.com/apache/wicket/pull/253#discussion_r159286873
--- Diff: wicket-core/src/main/java/org/apache/wicket/ajax/AbstractDefaultAjaxBehavior.java ---
@@ -106,8 +107,7 @@ public void renderHead(final Component component, final IHeaderResponse response
RequestCycle requestCycle = component.getRequestCycle();
Url baseUrl = requestCycle.getUrlRenderer().getBaseUrl();
CharSequence ajaxBaseUrl = Strings.escapeMarkup(baseUrl.toString());
- response.render(JavaScriptHeaderItem.forScript("Wicket.Ajax.baseUrl=\"" + ajaxBaseUrl
- + "\";", "wicket-ajax-base-url"));
--- End diff --
Ah this is only for the id attribute - so ignore this comment.
---
[GitHub] wicket pull request #253: WICKET-6498 deferred javascript
Posted by solomax <gi...@git.apache.org>.
Github user solomax commented on a diff in the pull request:
https://github.com/apache/wicket/pull/253#discussion_r161220021
--- Diff: wicket-core/src/main/java/org/apache/wicket/markup/head/ResourceAggregator.java ---
@@ -337,24 +338,27 @@ private void renderCombinedEventScripts()
if (combinedScript.length() > 0)
{
combinedScript.append("\nWicket.Event.publish(Wicket.Event.Topic.AJAX_HANDLERS_BOUND);");
- getRealResponse().render(
- OnDomReadyHeaderItem.forScript(combinedScript.append('\n').toString()));
+ getRealResponse().render(OnDomReadyHeaderItem.forScript(combinedScript.append('\n')));
}
combinedScript.setLength(0);
+ String lastId = null;
--- End diff --
Would appreciate review on this commit, especially this part ....
---
[GitHub] wicket issue #253: WICKET-6498 deferred javascript
Posted by svenmeier <gi...@git.apache.org>.
Github user svenmeier commented on the issue:
https://github.com/apache/wicket/pull/253
There aren't many test to check - the new pull request does not change the current Ajax implementation at all :p. IMHO that's a huge advantage.
---
[GitHub] wicket pull request #253: WICKET-6498 deferred javascript
Posted by solomax <gi...@git.apache.org>.
Github user solomax commented on a diff in the pull request:
https://github.com/apache/wicket/pull/253#discussion_r161121472
--- Diff: wicket-core/src/main/java/org/apache/wicket/ajax/AbstractDefaultAjaxBehavior.java ---
@@ -106,8 +107,7 @@ public void renderHead(final Component component, final IHeaderResponse response
RequestCycle requestCycle = component.getRequestCycle();
Url baseUrl = requestCycle.getUrlRenderer().getBaseUrl();
CharSequence ajaxBaseUrl = Strings.escapeMarkup(baseUrl.toString());
- response.render(JavaScriptHeaderItem.forScript("Wicket.Ajax.baseUrl=\"" + ajaxBaseUrl
- + "\";", "wicket-ajax-base-url"));
--- End diff --
this missing `id` attribute breaks `org.apache.wicket.util.tester.BaseWicketTester.getWicketAjaxBaseUrlEncodedInLastResponse()` ....
Will set it back if there are no better solutions ...
---