This repository has been archived by the owner. It is now read-only.

Moving core.js loading into separate method #981

Closed
wants to merge 4 commits into
from

Conversation

Projects
None yet
4 participants
@danyaPostfactum
Contributor

danyaPostfactum commented Mar 12, 2012

It is bad idea to merge core.js and motools loading.
I sure it is nesessary to separate this actions into different methods.

Im working at my plugin, wich re-registers such methods as behavior.keepalive, behavior.formvalidation etc,
This plugin allows to avoid mootools requirement, all my scripts uses jQuery.
Its ok, but I can't replace core.js. Moreover, merging mootools requirement with core.js creates big problems for me.

I don't like jQuery, MooTools is more beauty for me, BUT I'm using extensions, wich works on jQuery, and I'm forcet to use this js library and refuse from MooTools die to site weight optimisation.

This plugin works great in joomla 1.7, but in joomla 2.5 I cannot get rid of mootools due to core.js and framework merging.

Please separete this methods, I thing it is a good idea.

@realityking

This comment has been minimized.

Show comment
Hide comment
@realityking

realityking Mar 12, 2012

Contributor

Could you explAin your reasoning? We just removed the JHtml::core() method.

Also this is not backwards compatible, there's a ton of code relying on core.js being loaded when MooTools is.

Contributor

realityking commented Mar 12, 2012

Could you explAin your reasoning? We just removed the JHtml::core() method.

Also this is not backwards compatible, there's a ton of code relying on core.js being loaded when MooTools is.

@realityking

This comment has been minimized.

Show comment
Hide comment
@realityking

realityking Mar 12, 2012

Contributor

I just saw you send me an earlier, I'll respond in the morning ;)

Contributor

realityking commented Mar 12, 2012

I just saw you send me an earlier, I'll respond in the morning ;)

@danyaPostfactum

This comment has been minimized.

Show comment
Hide comment
@danyaPostfactum

danyaPostfactum Mar 12, 2012

Contributor

Ok, so I offer to call JHtml::_('behavior.core') from framework() method.
This way doesnt break the compatibility and allows to override core.js in plugins

But, JHtml::core usage must be replaced with JHtml::('behavior.core'), not with JHtml::('behavior.framework') ( to prevent mootools loading when we only need to core.js.

So, core.js will be loaded in any way, wether we call JHtml::('behavior.core'), JHtml::('behavior.framework') or JHtml::script()

But if extension needs only core.js, it calls JHtml::('behavior.core'), if needs mootools , it calls JHtml::('behavior.framework')

Is it possible to make these changes ?

Contributor

danyaPostfactum commented Mar 12, 2012

Ok, so I offer to call JHtml::_('behavior.core') from framework() method.
This way doesnt break the compatibility and allows to override core.js in plugins

But, JHtml::core usage must be replaced with JHtml::('behavior.core'), not with JHtml::('behavior.framework') ( to prevent mootools loading when we only need to core.js.

So, core.js will be loaded in any way, wether we call JHtml::('behavior.core'), JHtml::('behavior.framework') or JHtml::script()

But if extension needs only core.js, it calls JHtml::('behavior.core'), if needs mootools , it calls JHtml::('behavior.framework')

Is it possible to make these changes ?

@realityking

This comment has been minimized.

Show comment
Hide comment
@realityking

realityking Mar 13, 2012

Contributor

First of all I'd be very interested in your jQuery plug-in. Do you host it somewhere?

Now back to topic.

The code in core.js has depended on MooTools at least since Joomla 1.6, also at least since 1.6 JHtmlBehavior::framework() has loaded core.js, this used to be done by calling JHtml::core(). I just simplified the code since calling either one used to load both files.

I don't quite get what your concern is? That some extension the uses parts of core.js will call JHtml::_('behavior.framework')? How is it going to help you if we accept this pull request, JHtmlBeavhior::core() would still include MooTools.

I'm not saying I'm opposed to it, I'd just like to understand the problem first.

Contributor

realityking commented Mar 13, 2012

First of all I'd be very interested in your jQuery plug-in. Do you host it somewhere?

Now back to topic.

The code in core.js has depended on MooTools at least since Joomla 1.6, also at least since 1.6 JHtmlBehavior::framework() has loaded core.js, this used to be done by calling JHtml::core(). I just simplified the code since calling either one used to load both files.

I don't quite get what your concern is? That some extension the uses parts of core.js will call JHtml::_('behavior.framework')? How is it going to help you if we accept this pull request, JHtmlBeavhior::core() would still include MooTools.

I'm not saying I'm opposed to it, I'd just like to understand the problem first.

@danyaPostfactum

This comment has been minimized.

Show comment
Hide comment
@danyaPostfactum

danyaPostfactum Mar 13, 2012

Contributor

Ok, I have learned this problem deeper. I think it is possible to change a little bit of code without any combatibiltity problem, and with satisfaction of my interests.

First, what I think about usage of core.js and mootools framework:
If extension needs Joomla js object, it says behavior.core , If extension needs mootools framework (because of it uses $ function or addEvent for example) it says behavior.framework. If extension needs both of these things it calls both of behaviors. Extension shouldnt care how some of behaviors impements - may be it uses dojo or jquery, it doesnt matter.
Extension just calls behavior.core and gets Joomla object.
That is my point of view of how it should be ideally.
Due to backwards compatibility behavior.framework should call core.js anyway.

Second, to avoid recursion in calls of framework and core each other (framework calls core, and core calls framework and so on) we need to break this dependence. Because of we cannot remove the call of core in framework method (due to b.comp.) we need to rewrite core.js in pure js. Its very easy, there are two only methods which uses mootools, and I have rewritten it (with help of js master) already.

In future, when we get rid of the core.js call from behavior.framework, we will be able to use mootools in core.js (because of there will not be double-dependence of methods)

Finally, what we get:

  1. Logically correct way to load mootools.js and core.js
  2. Possibility of overriding core.js call. So, in most cases, core extensions such as com_content, com_contact, com_users will not call mootools directly, they will only call keepalive, tooltip etc behaviors, and this gives us possibility to implement this behaviors with usage of any js library/framework you wish, just using joomla plugin.

But, I don't how did you replace JHtml::core in CMS. I guess JHtml::('behavior.framework') ? But frontend parts of com_content, com_contact etc havent need to mootools, they dont use $, addEvent etc, only they need is core.js and other behaviors (tooltip, keepalive and so on)
Is it possible to change code in at least frontend part ? For now, in the last release of CMS, there over 10 calls of JHtml::core() - it so easy to change it to JHtml::
('behavior.core').
Without this change, my plugin will not give a full effect - mootools will be loading but will not be in use.
Or where can I try to discuss this problem ?

I will place my plugin here. on github, but i havent yet have deal with it, so it takes a time :)
Уфф...

Contributor

danyaPostfactum commented Mar 13, 2012

Ok, I have learned this problem deeper. I think it is possible to change a little bit of code without any combatibiltity problem, and with satisfaction of my interests.

First, what I think about usage of core.js and mootools framework:
If extension needs Joomla js object, it says behavior.core , If extension needs mootools framework (because of it uses $ function or addEvent for example) it says behavior.framework. If extension needs both of these things it calls both of behaviors. Extension shouldnt care how some of behaviors impements - may be it uses dojo or jquery, it doesnt matter.
Extension just calls behavior.core and gets Joomla object.
That is my point of view of how it should be ideally.
Due to backwards compatibility behavior.framework should call core.js anyway.

Second, to avoid recursion in calls of framework and core each other (framework calls core, and core calls framework and so on) we need to break this dependence. Because of we cannot remove the call of core in framework method (due to b.comp.) we need to rewrite core.js in pure js. Its very easy, there are two only methods which uses mootools, and I have rewritten it (with help of js master) already.

In future, when we get rid of the core.js call from behavior.framework, we will be able to use mootools in core.js (because of there will not be double-dependence of methods)

Finally, what we get:

  1. Logically correct way to load mootools.js and core.js
  2. Possibility of overriding core.js call. So, in most cases, core extensions such as com_content, com_contact, com_users will not call mootools directly, they will only call keepalive, tooltip etc behaviors, and this gives us possibility to implement this behaviors with usage of any js library/framework you wish, just using joomla plugin.

But, I don't how did you replace JHtml::core in CMS. I guess JHtml::('behavior.framework') ? But frontend parts of com_content, com_contact etc havent need to mootools, they dont use $, addEvent etc, only they need is core.js and other behaviors (tooltip, keepalive and so on)
Is it possible to change code in at least frontend part ? For now, in the last release of CMS, there over 10 calls of JHtml::core() - it so easy to change it to JHtml::
('behavior.core').
Without this change, my plugin will not give a full effect - mootools will be loading but will not be in use.
Or where can I try to discuss this problem ?

I will place my plugin here. on github, but i havent yet have deal with it, so it takes a time :)
Уфф...

@@ -58,13 +58,33 @@ public static function framework($extras = false, $debug = null)
}
JHtml::_('script', 'system/mootools-' . $type . '.js', false, true, false, false, $debug);
- JHtml::_('script', 'system/core.js', false, true);

This comment has been minimized.

@danyaPostfactum

danyaPostfactum Mar 14, 2012

Contributor

Remove direct loading of core.js, let ::core() method do it

@danyaPostfactum

danyaPostfactum Mar 14, 2012

Contributor

Remove direct loading of core.js, let ::core() method do it

@@ -82,7 +102,7 @@ public static function caption($selector = 'img.caption')
}
// Include MooTools framework
- self::framework();

This comment has been minimized.

@danyaPostfactum

danyaPostfactum Mar 14, 2012

Contributor

May be somebody wants to re-register behavior.framework (for loading from CDN for examle) , so, let it use his overriding method

@danyaPostfactum

danyaPostfactum Mar 14, 2012

Contributor

May be somebody wants to re-register behavior.framework (for loading from CDN for examle) , so, let it use his overriding method

self::$loaded[__METHOD__][$type] = true;
return;
}
/**
+ * Method to load the Joomla core behavior

This comment has been minimized.

@danyaPostfactum

danyaPostfactum Mar 14, 2012

Contributor

core.js should not use MooTools (at this moment) to prevent recursive calls of framework and core each other. In future, extensions should call directly ::core() method when they needs Joomla object, so we will remove ::core() call from framework method, and we will get possibility to use mootools in core.js

@danyaPostfactum

danyaPostfactum Mar 14, 2012

Contributor

core.js should not use MooTools (at this moment) to prevent recursive calls of framework and core each other. In future, extensions should call directly ::core() method when they needs Joomla object, so we will remove ::core() call from framework method, and we will get possibility to use mootools in core.js

@@ -124,35 +124,41 @@ Joomla.checkAll = function(checkbox, stub) {
* @return void
*/
Joomla.renderMessages = function(messages) {

This comment has been minimized.

@danyaPostfactum

danyaPostfactum Mar 14, 2012

Contributor

Just pure javascript, it so easy :)

@danyaPostfactum

danyaPostfactum Mar 14, 2012

Contributor

Just pure javascript, it so easy :)

@danyaPostfactum

This comment has been minimized.

Show comment
Hide comment
@danyaPostfactum

danyaPostfactum Mar 14, 2012

Contributor

So, what do you think about it?
I want to create fast and lightweight sites, wich dont load tons of javascript. I want to give the availability to web-developers to use jquery correctly without loading mootools if they dont wont to use it.

All changes i offered will not affect outside Htmlbehavior file

Contributor

danyaPostfactum commented Mar 14, 2012

So, what do you think about it?
I want to create fast and lightweight sites, wich dont load tons of javascript. I want to give the availability to web-developers to use jquery correctly without loading mootools if they dont wont to use it.

All changes i offered will not affect outside Htmlbehavior file

@realityking

This comment has been minimized.

Show comment
Hide comment
@realityking

realityking Mar 14, 2012

Contributor

I'll respond in a bit, between JSST and our state parliament disbanding itself today I'm kept pretty busy ;)

Contributor

realityking commented Mar 14, 2012

I'll respond in a bit, between JSST and our state parliament disbanding itself today I'm kept pretty busy ;)

@realityking

This comment has been minimized.

Show comment
Hide comment
@realityking

realityking Mar 16, 2012

Contributor

Haven't completely thought this trough but two three that came to my mind reading trough it:

  1. We'll have to add the JHtmlBeavhior::core() method, no doubt about it. We'll just have to add another file when we start extending MT objects (I have a rough cut of Request.Joomla laying around that was supposed to go into core.js)
  2. Can you really override JHtmlBehavior partly? So far I've alway overridden the complete JHtmlBehavior class - but then I've never tried it differently. I kinda like the self::framework() calls. That whole JHtml::_() method hides valuable information from IDEs so it should be avoided where possible.
  3. I'm very much against removing the MooTools code from core.js. I'd actually prefer if we could move some of the smaller files (e.g. multiselect) into core.js so we don't have to load that many files. But even if we can't do that, we got a framework in the core, why don't use it?

We should probably deprecate that JHtmlBehavior::framework() loads the core.js file. That will mean we'll have to add quite a bit of code but I really don't wanna be stuck in no framework mode in core.js.

Contributor

realityking commented Mar 16, 2012

Haven't completely thought this trough but two three that came to my mind reading trough it:

  1. We'll have to add the JHtmlBeavhior::core() method, no doubt about it. We'll just have to add another file when we start extending MT objects (I have a rough cut of Request.Joomla laying around that was supposed to go into core.js)
  2. Can you really override JHtmlBehavior partly? So far I've alway overridden the complete JHtmlBehavior class - but then I've never tried it differently. I kinda like the self::framework() calls. That whole JHtml::_() method hides valuable information from IDEs so it should be avoided where possible.
  3. I'm very much against removing the MooTools code from core.js. I'd actually prefer if we could move some of the smaller files (e.g. multiselect) into core.js so we don't have to load that many files. But even if we can't do that, we got a framework in the core, why don't use it?

We should probably deprecate that JHtmlBehavior::framework() loads the core.js file. That will mean we'll have to add quite a bit of code but I really don't wanna be stuck in no framework mode in core.js.

@danyaPostfactum

This comment has been minimized.

Show comment
Hide comment
@danyaPostfactum

danyaPostfactum Mar 17, 2012

Contributor

I'm glad to see you agree with some of my thoughts :)

You asked: 'Can you really override JHtmlBehavior partly?'
Yes, there is the way: JHtml::register('behavior.jquery', array('myClass', 'jquery')). It works.
And JHtml::register('behavior.framework', array('myClass', 'framework')) - it works too.
For now, if I need to override behavior.framework ( I want to load it from Google CDN, and I want to select wich parts of more.js I want to load, because I dont want to load this monsterous file), I have to override the whole behavior.php, due to self::framework() calls.
But if you don't want to change this, ok, it is not a big problem :)

I'm not against using MooTools code in core.js, but we cant use it, until we remove core.js loading in framework method.

If we will call core.js in framework method, and will call framework in core method, we will get recursion.

So we have to remove core.js loading in framework method, or we have to remove framework call in core method.

Due to backward compatibilty we are not allowed to do first thing, so lets do second one.
As you see, using pure js is not more complex then using MooTools in our case. Function renderMessages is still clear and simple.

After removing core.js loading in framework method, we will get availability to use MT in core.js again.

So, I must get back self:framework() lines in my patch, do I ?

Contributor

danyaPostfactum commented Mar 17, 2012

I'm glad to see you agree with some of my thoughts :)

You asked: 'Can you really override JHtmlBehavior partly?'
Yes, there is the way: JHtml::register('behavior.jquery', array('myClass', 'jquery')). It works.
And JHtml::register('behavior.framework', array('myClass', 'framework')) - it works too.
For now, if I need to override behavior.framework ( I want to load it from Google CDN, and I want to select wich parts of more.js I want to load, because I dont want to load this monsterous file), I have to override the whole behavior.php, due to self::framework() calls.
But if you don't want to change this, ok, it is not a big problem :)

I'm not against using MooTools code in core.js, but we cant use it, until we remove core.js loading in framework method.

If we will call core.js in framework method, and will call framework in core method, we will get recursion.

So we have to remove core.js loading in framework method, or we have to remove framework call in core method.

Due to backward compatibilty we are not allowed to do first thing, so lets do second one.
As you see, using pure js is not more complex then using MooTools in our case. Function renderMessages is still clear and simple.

After removing core.js loading in framework method, we will get availability to use MT in core.js again.

So, I must get back self:framework() lines in my patch, do I ?

@@ -162,8 +168,8 @@ Joomla.renderMessages = function(messages) {
* @return void
*/
Joomla.removeMessages = function() {
- var children = $$('#system-message-container > *');
- children.destroy();
+ var container = document.getElementById('system-message-container'), child;

This comment has been minimized.

@WebMechanic

WebMechanic Mar 25, 2012

Contributor

why travel the DOM?
document.getElementById('system-message-container').innerHTML='';

@WebMechanic

WebMechanic Mar 25, 2012

Contributor

why travel the DOM?
document.getElementById('system-message-container').innerHTML='';

This comment has been minimized.

@danyaPostfactum

danyaPostfactum Mar 25, 2012

Contributor

I don't know. I asked on javascript.ru to help me to "decode" mootools code into pure js , and professional js-coder helped me.
I also offered innerHTML = '', but he sayd "removeChild() would be better" ....

@danyaPostfactum

danyaPostfactum Mar 25, 2012

Contributor

I don't know. I asked on javascript.ru to help me to "decode" mootools code into pure js , and professional js-coder helped me.
I also offered innerHTML = '', but he sayd "removeChild() would be better" ....

This comment has been minimized.

@WebMechanic

WebMechanic Mar 25, 2012

Contributor

and did that professional js-coder also explain to you why it is suposed to be "better"?
It's DOM Level1, granted, but that's about it :)
Not only will this save a variable, a loop, and memory, innerHTML is also much faster which is why virtually any JS lib is using it internally to speed up these kind of "mass" DOM manipulation.
+1 that you ripped off the dependency on MooTools from renderMessage!

edit: this was my personal take on ripping it off: https://gist.github.com/2199473

@WebMechanic

WebMechanic Mar 25, 2012

Contributor

and did that professional js-coder also explain to you why it is suposed to be "better"?
It's DOM Level1, granted, but that's about it :)
Not only will this save a variable, a loop, and memory, innerHTML is also much faster which is why virtually any JS lib is using it internally to speed up these kind of "mass" DOM manipulation.
+1 that you ripped off the dependency on MooTools from renderMessage!

edit: this was my personal take on ripping it off: https://gist.github.com/2199473

@danyaPostfactum

This comment has been minimized.

Show comment
Hide comment
@danyaPostfactum

danyaPostfactum Mar 26, 2012

Contributor

So, what about merging my patches?

Contributor

danyaPostfactum commented Mar 26, 2012

So, what about merging my patches?

@LouisLandry

This comment has been minimized.

Show comment
Hide comment
@LouisLandry

LouisLandry Oct 9, 2012

Contributor

@danyaPostfactum This is no longer mergeable. Can you rebase this so we can take another look? @realityking would you have another look at this and talk it through with the CMS folks as well so we can make a call?

I'm going to close this for now. @danyaPostfactum once you have rebased this so that it will cleanly merge again please re-open it so we can finish the discussion. My apologies for the delay in getting a decision made on it.

Contributor

LouisLandry commented Oct 9, 2012

@danyaPostfactum This is no longer mergeable. Can you rebase this so we can take another look? @realityking would you have another look at this and talk it through with the CMS folks as well so we can make a call?

I'm going to close this for now. @danyaPostfactum once you have rebased this so that it will cleanly merge again please re-open it so we can finish the discussion. My apologies for the delay in getting a decision made on it.

@LouisLandry LouisLandry closed this Oct 9, 2012

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.