Re: [Wikitech-l] Using prototypical inheritance in dynamically loaded ResourceLoader modules
* Krinkle krinklem...@gmail.com [Mon, 19 Mar 2012 14:32:51 +0100]:
Converted all of for..in into $.each(). The most funny thing is that
$.each() did not work correctly with sparse arrays [], walking with
undefs between real elements. While for..in used to work fine
(FF,Chrome,IE8,IE9). So I had to convert sparse arrays into sparse
objects {}, just as you recommended. This way $.each() works, however it
also creates new context (different this) so I've had to use var
myself = this more often than it used to be.
The most pailful part was to pass control to the main module after edit
module was abruptly terminated (without any warnings in Chrome console).
I managed to call the main module method directly from edit module. Not
a very nice thing, but at least it works. Who would know that such
potentially easy thing as refactoring one js class into two
ResourceLoader modules can be that much painful. I used to dynamically
load jQuery for custom MediaWiki 1.15 installation via LABjs and don't
remember such problems back then.
Dmitriy
___
Wikitech-l mailing list
Wikitech-l@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Re: [Wikitech-l] Using prototypical inheritance in dynamically loaded ResourceLoader modules
Hi! I've tweaked my code few times, trying to make it simpler: got rid of some closure calls and left only one dynamic module load at client-side. http://pastebin.com/UxyifLmx http://pastebin.com/q3Tm6Ajd http://pastebin.com/4emMDBS6 Still, it gives me headaches, because mw.loader.using( 'ext.jqgmap.edit' ,function(...)) does not actually execute the loaded module's code even when the callback function is fired. This means that additional mw.jqgmap prototypes defined in 'ext.jqgmap.edit' are not available when actual object instances are created in function createMapControllers(), thus throws an error. When I add explicit call to dummy function loadEditPrototypes() defined in 'ext.jqgmap.edit', the module is executed, but the further scipts execution abruptly ends without any error in Chrome console. To me it seems that ResourceLoader tries to be too smart not actually executing the code just before .using() callback. How does it figure out that I defined function loadEditPrototypes() in 'ext.jqgmap.edit' module? That kind of 'automagic' is tricky. Why not to have something like $wgAutoloadClasses but for ResourceLoader? Or, even to be able to really execute module on load. And why does it stop execution path is a puzzle to me.. Dmitriy ___ Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Re: [Wikitech-l] Using prototypical inheritance in dynamically loaded ResourceLoader modules
On Mon, 19 Mar 2012 00:40:54 -0700, Dmitriy Sintsov ques...@rambler.ru
wrote:
Hi!
I've tweaked my code few times, trying to make it simpler: got rid of
some closure calls and left only one dynamic module load at client-side.
http://pastebin.com/UxyifLmx
http://pastebin.com/q3Tm6Ajd
http://pastebin.com/4emMDBS6
Still, it gives me headaches, because mw.loader.using( 'ext.jqgmap.edit'
,function(...)) does not actually execute the loaded module's code even
when the callback function is fired. This means that additional
mw.jqgmap prototypes defined in 'ext.jqgmap.edit' are not available when
actual object instances are created in function createMapControllers(),
thus throws an error. When I add explicit call to dummy function
loadEditPrototypes() defined in 'ext.jqgmap.edit', the module is
executed, but the further scipts execution abruptly ends without any
error in Chrome console.
To me it seems that ResourceLoader tries to be too smart not actually
executing the code just before .using() callback. How does it figure out
that I defined function loadEditPrototypes() in 'ext.jqgmap.edit'
module? That kind of 'automagic' is tricky. Why not to have something
like $wgAutoloadClasses but for ResourceLoader? Or, even to be able to
really execute module on load. And why does it stop execution path is a
puzzle to me..
Autoloading classes is not possible. Even if every browser supported
getters and we could use them to dynamically load classes, this would
require synchronous http calls. Which are absolutely HORRIBLE because they
block the entire JS thread and in addition typically even block the
browser's UI.
Dmitriy
You shouldn't be dropping the closure, you don't want local things in the
global scope. You also shouldn't be using the global $ and mw directly.
Everything should be using a pattern like:
( function( $, mw ) {
} )( jQuery, mediaWiki );
var jqgmap = [];
for ( var mapIndex in jqgmap ) {
This is VERY bad JavaScript coding practice. Please use $.each().
$('div class=jqgmap_code id=jqgmap_code' + this.Idx + ''+
'input type=checkbox/input' + mw.msg( 'jqgmap-show-code' )
+
'input type=checkbox/input' + mw.msg( 'jqgmap-show-code' )
+
'div class=jqgmap_preview/div' +
'div class=jqgmap_preview/div' +
'/div')
I should really start poking people about this one. `div . wfMsg(
'foo' ) . /div` is bad in PHP code, and it's just as bad inside JS.
You should be creating your html with NO + concatenation, and then using a
.find() and .text() to insert things where they belong. That, or use
multiple $() calls with .append() to create the full structure.
Likewise you shouldn;t be inserting this.Idx that way into the attribute.
It should be added with something like .attr( jqgmap_code + this.Idx );
--
~Daniel Friesen (Dantman, Nadir-Seen-Fire) [http://daniel.friesen.name]
___
Wikitech-l mailing list
Wikitech-l@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Re: [Wikitech-l] Using prototypical inheritance in dynamically loaded ResourceLoader modules
* Daniel Friesen li...@nadir-seen-fire.com [Mon, 19 Mar 2012 01:35:21
-0700]:
Autoloading classes is not possible. Even if every browser supported
getters and we could use them to dynamically load classes, this would
require synchronous http calls. Which are absolutely HORRIBLE because
they
block the entire JS thread and in addition typically even block the
browser's UI.
How does ResourceLoader figures that function loadEditPrototypes() was
defined in 'ext.jqgmap.edit' when I call this function from
'ext.jqgmap'? And if there was no extra http call, then why the list of
scripts in Chrome debugger does not include 'JQGmap.edit.js' among the
scripts before mw.loader.using() and loadEditPrototypes() call was
performed?
You shouldn't be dropping the closure, you don't want local things in
the
global scope. You also shouldn't be using the global $ and mw
directly.
Prototypes are not local things. They are carried out with function they
belong to. I define few prototypes in 'ext.jqgmap.view' then I want to
add more prototypes in 'ext.jqgmap.edit'. But not always, only when the
edit mode is active. However you are right about minor local variables.
Everything should be using a pattern like:
( function( $, mw ) {
} )( jQuery, mediaWiki );
Ok, I'll try the closure call with two arguments in every module, didn't
think about that. I'll report if that will fix the abrupt termination of
module chain execution.
var jqgmap = [];
for ( var mapIndex in jqgmap ) {
This is VERY bad JavaScript coding practice. Please use $.each().
I know that I can use $.each() here. I don't do that for few reasons:
for..in is easier to debug (step by step key) and also because for..in
was problematic only for objects {}, not for arrays [] AFAIK. However I
might be wrong. Maybe I'll switch to $.each() however the code was
working with for..in perfectly well before I refactored it into multiple
modules. There was only one module. However it was growing so I had to
split it into view and edit submodules.
$('div class=jqgmap_code id=jqgmap_code' + this.Idx + ''+
'input type=checkbox/input' + mw.msg(
'jqgmap-show-code'
)
+
'input type=checkbox/input' + mw.msg(
'jqgmap-show-code'
)
+
'div class=jqgmap_preview/div' +
'div class=jqgmap_preview/div' +
'/div')
I should really start poking people about this one. `div . wfMsg(
'foo' ) . /div` is bad in PHP code, and it's just as bad inside
JS.
You should be creating your html with NO + concatenation, and then
using
a
.find() and .text() to insert things where they belong. That, or use
multiple $() calls with .append() to create the full structure.
Likewise you shouldn;t be inserting this.Idx that way into the
attribute.
It should be added with something like .attr( jqgmap_code + this.Idx
);
I might consider refactoring of jQuery DOM nodes creation, however the
loading of 'ext.jqgmap.edit' module does not work now so I cannot
continue until I'll figure out why. This code is not ideal however it's
not the reason of the fault. This code was working perfectly well before
splitting into multiple modules and introducing
mw.loader.using('ext.jqgmap.edit',function(){...});
Dmitriy
___
Wikitech-l mailing list
Wikitech-l@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Re: [Wikitech-l] Using prototypical inheritance in dynamically loaded ResourceLoader modules
* Daniel Friesen li...@nadir-seen-fire.com [Mon, 19 Mar 2012 01:35:21
-0700]:
( function( $, mw ) {
} )( jQuery, mediaWiki );
I modified all of three modules: main, view and edit module to the
recommended pattern.
http://pastebin.com/1kS6EyUu
http://pastebin.com/WQzBTw6W
http://pastebin.com/UqpTAvZ8
However, the execution still stops after the following call:
mw.jqgmap.loadEdit();
performed in main module. When I comment out that call, execution of
further scripts does not terminate: however extra prototypes defined in
'ext.jqgmap.edit' become unavailable; thus MapController and
MarkerController instances do not work correctly in edit mode.
Still cannot dynamically load 'ext.jqgmap.edit' successfully..
Maybe I'll temporary re-unite the modules into monolithic one, otherwise
I risk to ruin the timeline of project. Too bad I wasn't able to defeat
the trouble.
Dmitriy
___
Wikitech-l mailing list
Wikitech-l@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Re: [Wikitech-l] Using prototypical inheritance in dynamically loaded ResourceLoader modules
On Mon, 19 Mar 2012 02:07:54 -0700, Dmitriy Sintsov ques...@rambler.ru
wrote:
* Daniel Friesen li...@nadir-seen-fire.com [Mon, 19 Mar 2012 01:35:21
-0700]:
Autoloading classes is not possible. Even if every browser supported
getters and we could use them to dynamically load classes, this would
require synchronous http calls. Which are absolutely HORRIBLE because
they
block the entire JS thread and in addition typically even block the
browser's UI.
How does ResourceLoader figures that function loadEditPrototypes() was
defined in 'ext.jqgmap.edit' when I call this function from
'ext.jqgmap'? And if there was no extra http call, then why the list of
scripts in Chrome debugger does not include 'JQGmap.edit.js' among the
scripts before mw.loader.using() and loadEditPrototypes() call was
performed?
RL doesn't know anything about loadEditPrototypes, I don't know what would
make you think that it does.
I can't really debug and say what's wrong without your code actually
running somewhere.
You shouldn't be dropping the closure, you don't want local things in
the
global scope. You also shouldn't be using the global $ and mw
directly.
Prototypes are not local things. They are carried out with function they
belong to. I define few prototypes in 'ext.jqgmap.view' then I want to
add more prototypes in 'ext.jqgmap.edit'. But not always, only when the
edit mode is active. However you are right about minor local variables.
Everything should be using a pattern like:
( function( $, mw ) {
} )( jQuery, mediaWiki );
Ok, I'll try the closure call with two arguments in every module, didn't
think about that. I'll report if that will fix the abrupt termination of
module chain execution.
var jqgmap = [];
for ( var mapIndex in jqgmap ) {
This is VERY bad JavaScript coding practice. Please use $.each().
I know that I can use $.each() here. I don't do that for few reasons:
for..in is easier to debug (step by step key) and also because for..in
was problematic only for objects {}, not for arrays [] AFAIK. However I
might be wrong. Maybe I'll switch to $.each() however the code was
working with for..in perfectly well before I refactored it into multiple
modules. There was only one module. However it was growing so I had to
split it into view and edit submodules.
Completely wrong.
for..in is object iteration. It is not to be used for array iteration.
When you use for..in to iterate over an array you're actually iterating
over object keys. Unset things may be left out, if there are any
properties set you'll iterate over those when you shouldn't, if someone
sets something on Array.prototype. (eg: Someone implements .forEach in an
older browser) you'll iterate over that as well, and it's perfectly valid
for a browser to iterate over the keys in an order that's different from
the actual order of items in the array.
$('div class=jqgmap_code id=jqgmap_code' + this.Idx + ''+
'input type=checkbox/input' + mw.msg(
'jqgmap-show-code'
)
+
'input type=checkbox/input' + mw.msg(
'jqgmap-show-code'
)
+
'div class=jqgmap_preview/div' +
'div class=jqgmap_preview/div' +
'/div')
I should really start poking people about this one. `div . wfMsg(
'foo' ) . /div` is bad in PHP code, and it's just as bad inside
JS.
You should be creating your html with NO + concatenation, and then
using
a
.find() and .text() to insert things where they belong. That, or use
multiple $() calls with .append() to create the full structure.
Likewise you shouldn;t be inserting this.Idx that way into the
attribute.
It should be added with something like .attr( jqgmap_code + this.Idx
);
I might consider refactoring of jQuery DOM nodes creation, however the
loading of 'ext.jqgmap.edit' module does not work now so I cannot
continue until I'll figure out why. This code is not ideal however it's
not the reason of the fault. This code was working perfectly well before
splitting into multiple modules and introducing
mw.loader.using('ext.jqgmap.edit',function(){...});
Dmitriy
--
~Daniel Friesen (Dantman, Nadir-Seen-Fire) [http://daniel.friesen.name]
___
Wikitech-l mailing list
Wikitech-l@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Re: [Wikitech-l] Using prototypical inheritance in dynamically loaded ResourceLoader modules
offtopic
On Mon, Mar 19, 2012 at 9:35 AM, Daniel Friesen
li...@nadir-seen-fire.comwrote:
On Mon, 19 Mar 2012 00:40:54 -0700, Dmitriy Sintsov ques...@rambler.ru
wrote:
var jqgmap = [];
for ( var mapIndex in jqgmap ) {
This is VERY bad JavaScript coding practice. Please use $.each().
This is rather exaggerated. Even more when looking at that suggestion.
Arrays should, indeed, not be enumerated with a for-in loop. Arrays in JS
can only contain numeral indices, so they should simply be iterated with a
simple for-loop like this `for (i = 0; i myArr.length; i += 1) { .. }`
(maybe cache length for slight performance gain by reducing property
lookups).
Using $.each has overhead (+1+n function invocations). When given an array
it will do a simple for-loop with a counter. It has overhead of 1+n
additional function invocations and context creations. In most cases there
is no use for it. There is one case where it is handy and that's when you
specifically need a local context for the loop (being careful not to create
later-called functions inside the loop, risking all variables being in the
post-loop state). If you don't need a local scope for your loop, then using
$.each (or the native [].forEach in later browsers) is pointless as it only
has overhead of additional function invocations and lowering the position
in the scope chain.
When iterating over objects, however, (not arrays) then $.each is no better
than a for-in loop because (contrary to what some people think) it is not a
shortcut for for-in + if-hasOwn wrapper. When an object is passed, it
literally just does a plain for-in loop invoking the callback with the
value. jQuery does not support environments where someone extends the
native Object.prototype because it is considered harmful (an therefore
MediaWiki inherently does not support that either), so a plain for-in loop
over an object (excluding array objects) is perfectly fine according to our
conventions.
See also http://stackoverflow.com/a/1198447/319266
but so much for the good (and bad, evil) parts of javascript :D
-- Krinkle
___
Wikitech-l mailing list
Wikitech-l@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Re: [Wikitech-l] Using prototypical inheritance in dynamically loaded ResourceLoader modules
offtopic
On Mon, Mar 19, 2012 at 2:23 PM, Krinkle krinklem...@gmail.com wrote:
On Mon, Mar 19, 2012 at 9:35 AM, Daniel Friesen li...@nadir-seen-fire.com
wrote:
On Mon, 19 Mar 2012 00:40:54 -0700, Dmitriy Sintsov ques...@rambler.ru
wrote:
var jqgmap = [];
for ( var mapIndex in jqgmap ) {
This is VERY bad JavaScript coding practice. Please use $.each().
This is rather exaggerated. Even more when looking at that suggestion.
Arrays should, indeed, not be enumerated with a for-in loop. Arrays in JS
can only contain numeral indices, so they should simply be iterated with a
s/can/should only contain numeral indices. Arrays as just objects so they
can indeed
contain anything, and inherit functions. Also note that a for-in loop on
arrays will return
the keys as strings, not numbers:
`var a = ['foo', 'bar']; for (var b in a) {}; console.log(typeof b, b /*
string, 1 */);`
-- Krinkle
___
Wikitech-l mailing list
Wikitech-l@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Re: [Wikitech-l] Using prototypical inheritance in dynamically loaded ResourceLoader modules
On 19.03.2012 17:23, Krinkle wrote:
offtopic
On Mon, Mar 19, 2012 at 9:35 AM, Daniel Friesen
li...@nadir-seen-fire.comwrote:
On Mon, 19 Mar 2012 00:40:54 -0700, Dmitriy Sintsovques...@rambler.ru
wrote:
var jqgmap = [];
for ( var mapIndex in jqgmap ) {
This is VERY bad JavaScript coding practice. Please use $.each().
This is rather exaggerated. Even more when looking at that suggestion.
Arrays should, indeed, not be enumerated with a for-in loop. Arrays in JS
can only contain numeral indices, so they should simply be iterated with a
simple for-loop like this `for (i = 0; i myArr.length; i += 1) { .. }`
(maybe cache length for slight performance gain by reducing property
lookups).
My array is numeric but sparse,
myArr = [];
myArr[0] = a;
myArr[1] = b;
So I cannot just use incremental key iteration, at least existence of
element should be checked.
Using $.each has overhead (+1+n function invocations). When given an array
it will do a simple for-loop with a counter. It has overhead of 1+n
additional function invocations and context creations. In most cases there
is no use for it. There is one case where it is handy and that's when you
specifically need a local context for the loop (being careful not to create
later-called functions inside the loop, risking all variables being in the
post-loop state). If you don't need a local scope for your loop, then using
$.each (or the native [].forEach in later browsers) is pointless as it only
has overhead of additional function invocations and lowering the position
in the scope chain.
When iterating over objects, however, (not arrays) then $.each is no better
than a for-in loop because (contrary to what some people think) it is not a
shortcut for for-in + if-hasOwn wrapper. When an object is passed, it
literally just does a plain for-in loop invoking the callback with the
value. jQuery does not support environments where someone extends the
native Object.prototype because it is considered harmful (an therefore
MediaWiki inherently does not support that either), so a plain for-in loop
over an object (excluding array objects) is perfectly fine according to our
conventions.
See also http://stackoverflow.com/a/1198447/319266
but so much for the good (and bad, evil) parts of javascript :D
I was thinking about Daniel warnings on Array prototyping and thought
that proper iteration would check whether the prototype has the same
member, then do not include it (skip from) callback.
Dmitriy
___
Wikitech-l mailing list
Wikitech-l@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Re: [Wikitech-l] Using prototypical inheritance in dynamically loaded ResourceLoader modules
On Mon, 19 Mar 2012 06:23:13 -0700, Krinkle krinklem...@gmail.com wrote:
offtopic
On Mon, Mar 19, 2012 at 9:35 AM, Daniel Friesen
li...@nadir-seen-fire.comwrote:
On Mon, 19 Mar 2012 00:40:54 -0700, Dmitriy Sintsov ques...@rambler.ru
wrote:
var jqgmap = [];
for ( var mapIndex in jqgmap ) {
This is VERY bad JavaScript coding practice. Please use $.each().
This is rather exaggerated. Even more when looking at that suggestion.
There's nothing exaggerated about telling people not to use for..in to
loop over arrays. for..in is object iteration, period.
The $.each recommendation is because that IS our equivalent to foreach for
arrays.
Don't underestimate the developer distaste for writing stuff like this all
the time:
`for ( var i = 0, l = arr.length; i l; i++ ) { var arr2 = arr[i]; for (
var j = 0, ll = arr2.length; j ll; j++ ) { var item = arr2[j]; } }`
I never said not to use numeric for loops. But there's no reason to ditch
clean code.
Arrays should, indeed, not be enumerated with a for-in loop. Arrays in JS
can only contain numeral indices, so they should simply be iterated with
a
simple for-loop like this `for (i = 0; i myArr.length; i += 1) { .. }`
(maybe cache length for slight performance gain by reducing property
lookups).
Using $.each has overhead (+1+n function invocations). When given an
array
it will do a simple for-loop with a counter. It has overhead of 1+n
additional function invocations and context creations. In most cases
there
is no use for it. There is one case where it is handy and that's when you
specifically need a local context for the loop (being careful not to
create
later-called functions inside the loop, risking all variables being in
the
post-loop state). If you don't need a local scope for your loop, then
using
$.each (or the native [].forEach in later browsers) is pointless as it
only
has overhead of additional function invocations and lowering the position
in the scope chain.
When iterating over objects, however, (not arrays) then $.each is no
better
than a for-in loop because (contrary to what some people think) it is
not a
shortcut for for-in + if-hasOwn wrapper. When an object is passed, it
literally just does a plain for-in loop invoking the callback with the
value. jQuery does not support environments where someone extends the
native Object.prototype because it is considered harmful (an therefore
MediaWiki inherently does not support that either), so a plain for-in
loop
over an object (excluding array objects) is perfectly fine according to
our
conventions.
See also http://stackoverflow.com/a/1198447/319266
but so much for the good (and bad, evil) parts of javascript :D
-- Krinkle
Micro-optimizations like these are pointless. Most arrays have under 100
elements. No matter what type of loop you use the time taken for the
operation is negligible. More important is that the code is clean and easy
to read and write.
Unless you are working with 5000+ element arrays or working with really
low level code doing things like parsing and highlighting code there's no
real reason to micro-optimize.
If we cared about efficiency at that level. We wouldn't be using any of
jQuery's dom querying or dom creation functionality. dom operations are
practically the slowest thing there is to work with and jQuery just adds
overhead.
Btw, ;) for..in is actually the least efficient way to loop, the only
thing that is slower than for..in is for..in with hasOwnProperty:
http://jsperf.com/loop-testx
http://jsperf.com/loop-test-short
--
~Daniel Friesen (Dantman, Nadir-Seen-Fire) [http://daniel.friesen.name]
___
Wikitech-l mailing list
Wikitech-l@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Re: [Wikitech-l] Using prototypical inheritance in dynamically loaded ResourceLoader modules
On 18.03.2012 18:26, Dmitriy Sintsov wrote: replaced 'new MarkerController(...)' calls to 'new mw.jqgmap.MarkerController(...)' calls left from incomplete refactoring of early working (non-broken) revision, however refactored separate view / edit modules code still does not work, with the same js code being stopped without additional Chrome errors - so it hard to guess the reason what's broken in my js code. Dmitriy ___ Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
