librelist archives

« back to archive

code review request - require-tiny

code review request - require-tiny

From:
Tom Yandell
Date:
2011-05-05 @ 22:36
Not node.js related but anyway...

Michael: want to code review this for me?

https://github.com/tomyan/require-tiny/blob/master/src/require-tiny.js

No tests yet - is a dirty prototype :-) Is 2697 bytes, 933 bytes minified
and 568 bytes gzipped.

Tom

Re: [nug] code review request - require-tiny

From:
Michael Mathews
Date:
2011-05-06 @ 06:27
I suspect that you intend this as more of a code-sketch, than code to be 
reviewed, so I won't go over all the basic problems that make it 
unrunnable at the moment (I'm sure you can debug your own code) and stick 
to higher-level comments. Firstly I think syncRequire is an iffy 
proposition: it will succeed all day long until the server gets busy and 
then sometimes fail as the race conditions change. While it is 
deterministic in specific cases, in others it will appear to succeed 
randomly, depending on which asynchronous process finishes first and how 
the dependency tree is defined elsewhere. Secondly there doesn't appear to
be any code to deal with the case where the a module is async required, 
and so requested via HTTP, and then required again before that first HTTP 
request returns. gloader had the ability to track the lifecycle of a 
require/request, and could see this condition existed and therefore 
refrain from making the second HTTP request. tiny seems to just make two 
requests(?) 
and carry on. This all gets even more complicated if you add the case of 
an async require followed by (but before the async returns) a sync 
require. gloader had to deal with this case too. If I were being Agile I'd
suggest you ditch the syncRequire for the first release; it's a very hard 
thing to get right when it is combined with async requires.

-- 
Michael Mathews
On Thursday, 5 May 2011 at 23:36, Tom Yandell wrote: 
> Not node.js related but anyway...
> 
> Michael: want to code review this for me?
> 
> https://github.com/tomyan/require-tiny/blob/master/src/require-tiny.js 
> 
> No tests yet - is a dirty prototype :-) Is 2697 bytes, 933 bytes 
minified and 568 bytes gzipped. 
> 
> Tom 

Re: [nug] code review request - require-tiny

From:
Tom Yandell
Date:
2011-05-07 @ 06:10
On 6 May 2011 07:27, Michael Mathews <micmath@gmail.com> wrote:

> I suspect that you intend this as more of a code-sketch, than code to be
> reviewed, so I won't go over all the basic problems that make it unrunnable
> at the moment (I'm sure you can debug your own code) and stick to
> higher-level comments.
>

Indeed.


> Firstly I think syncRequire is an iffy proposition: it will succeed all day
> long until the server gets busy and then sometimes fail as the race
> conditions change. While it is deterministic in specific cases, in others it
> will appear to succeed randomly, depending on which asynchronous process
> finishes first and how the dependency tree is defined elsewhere.
>

Is this not the behaviour of require.js? It is (I think) intended for the
case where code is converted from a commonjs module. It is first scanned for
sync require calls (var a = require('a')), which are pulled up to the define
call level and a local require is passed in, effectively preloaded with a.
Can see that this could be fragile if used for other use cases though.


> Secondly there doesn't appear to be any code to deal with the case where
> the a module is async required, and so requested via HTTP, and then required
> again before that first HTTP request returns.
>

Good spot, will add this after I fix the code.


> gloader had the ability to track the lifecycle of a require/request, and
> could see this condition existed and therefore refrain from making the
> second HTTP r equest. tiny seems to just make two requests(?) and carry on.
> This all gets even more complicated if you add the case of an async require
> followed by (but before the async returns) a sync require. gloader had to
> deal with this case too. If I were being Agile I'd suggest you ditch
> the syncRequire for the first release; it's a very hard thing to get right
> when it is combined with async requires.
>

The sync require (perhaps misnamed) doesn't actually do any loading, it's
just there for the use case described above. I don't intend to do any actual
synchronous loading as it's not required by AMD and doesn't seem practical -
this was the same conclusion you guys came to with glow2 right?

Tom


>
> --
> Michael Mathews
>
> On Thursday, 5 May 2011 at 23:36, Tom Yandell wrote:
>
> Not node.js related but anyway...
>
> Michael: want to code review this for me?
>
> https://github.com/tomyan/require-tiny/blob/master/src/require-tiny.js
>
> No tests yet - is a dirty prototype :-) Is 2697 bytes, 933 bytes minified
> and 568 bytes gzipped.
>
> Tom
>
>
>

Re: [nug] code review request - require-tiny

From:
Michael Mathews
Date:
2011-05-07 @ 10:29
On Saturday, 7 May 2011 at 07:10, Tom Yandell wrote:

> 
> On 6 May 2011 07:27, Michael Mathews <micmath@gmail.com> wrote:
> > I suspect that you intend this as more of a code-sketch, than code to 
be reviewed, so I won't go over all the basic problems that make it 
unrunnable at the moment (I'm sure you can debug your own code) and stick 
to higher-level comments. 
> 
> Indeed.
> 
> >  Firstly I think syncRequire is an iffy proposition: it will succeed 
all day long until the server gets busy and then sometimes fail as the 
race conditions change. While it is deterministic in specific cases, in 
others it will appear to succeed randomly, depending on which asynchronous
process finishes first and how the dependency tree is defined elsewhere.
> > 
> 
> 
> Is this not the behaviour of require.js? It is (I think) intended for 
the case where code is converted from a commonjs module. It is first 
scanned for sync require calls (var a = require('a')), which are pulled up
to the define call level and a local require is passed in, effectively 
preloaded with a. Can see that this could be fragile if used for other use
cases though.

If that is a behaviour of requirejs I've never used it, nor would I. I 
prefer to have my code rewritten by the maintainer if it needs to be 
rewritten. But it wasn't clear at all from your code that you intended all
your "syncRequire" calls to be rewritten into simple requires. How would 
that happen exactly? Do you intend to implement that with actual code 
someday? How will such code handle more complicated (but perfectly valid) 
syntax involving calls to your syncRequire inside a nested scope?

define('x', { flip: 1 });
define('y', function() {
var x = { flip: 0; }
(function(){
var a,
x = y = { flip: 1? syncRequire('x').flip : 0 };
if (x.flip !== 1) throw 'fail';
})();
if (x.flip !== 0) throw 'fail';
});


It seems like it would be much more reliable for the author to simply 
remove the syncRequire and write something like this instead:

define('x', { flip: 1 });
define('y', ['x'], function(myX) {
var x = { flip: 0; }
(function(){
var a,
x = y = { flip: 1? myX.flip : 0 };
if (x.flip !== 1) throw 'fail';
})();
if (x.flip !== 0) throw 'fail';
});


I'd be very interested in seeing how you would automate something like 
that though. Certainly doing so correctly and staying "tiny" would be a 
challenge, in my estimation.

On the other hand, if you intended syncRequire to simply be a way to 
access an already loaded module, as I originally assumed, then it is 
either unnecessary or unreliable. In the case where the module is listed 
as a dependency (as in my second example above) it is unnecessary since 
the module is already guaranteed to be there and can be listed as one of 
the formal parameters to your builder function; In the case where it is 
not listed as a requirement (as in my first example) then it is not 
guaranteed to be there and so should be expected to sometimes be 
unavailable to use.
> 
> >  Secondly there doesn't appear to be any code to deal with the case 
where the a module is async required, and so requested via HTTP, and then 
required again before that first HTTP request returns.
> > 
> 
> 
> Good spot, will add this after I fix the code.
> 
> >  gloader had the ability to track the lifecycle of a require/request, 
and could see this condition existed and therefore refrain from making the
second HTTP r equest. tiny seems to just make two requests(?) and carry 
on. This all gets even more complicated if you add the case of an async 
require followed by (but before the async returns) a sync require. gloader
had to deal with this case too. If I were being Agile I'd suggest you 
ditch the syncRequire for the first release; it's a very hard thing to get
right when it is combined with async requires.
> > 
> 
> 
> The sync require (perhaps misnamed) doesn't actually do any loading, 
it's just there for the use case described above. I don't intend to do any
actual synchronous loading as it's not required by AMD and doesn't seem 
practical - this was the same conclusion you guys came to with glow2 
right? 
> 

Yes, that was the reason we ditched sync loading when it came time to 
rewrite gloader. And it's exactly why I'm suggesting you ditch syncRequire
from your own proposal now. For many reasons it is a raft of problems, 
without much (if any) benefit.

> 
> Tom
> 
> > 
> > -- 
> > Michael Mathews
> > On Thursday, 5 May 2011 at 23:36, Tom Yandell wrote:
> > > Not node.js related but anyway...
> > > 
> > > Michael: want to code review this for me?
> > > 
> > > https://github.com/tomyan/require-tiny/blob/master/src/require-tiny.js 
> > > 
> > > No tests yet - is a dirty prototype :-) Is 2697 bytes, 933 bytes 
minified and 568 bytes gzipped. 
> > > 
> > > Tom 
> > 
> > 
> 

Re: [nug] code review request - require-tiny

From:
Tom Yandell
Date:
2011-05-07 @ 12:54
On 7 May 2011 11:29, Michael Mathews <micmath@gmail.com> wrote:

> On Saturday, 7 May 2011 at 07:10, Tom Yandell wrote:
>
>
>
> On 6 May 2011 07:27, Michael Mathews <micmath@gmail.com> wrote:
>
>  I suspect that you intend this as more of a code-sketch, than code to be
> reviewed, so I won't go over all the basic problems that make it unrunnable
> at the moment (I'm sure you can debug your own code) and stick to
> higher-level comments.
>
>
> Indeed.
>
>
> Firstly I think syncRequire is an iffy proposition: it will succeed all day
> long until the server gets busy and then sometimes fail as the race
> conditions change. While it is deterministic in specific cases, in others it
> will appear to succeed randomly, depending on which asynchronous process
> finishes first and how the dependency tree is defined elsewhere.
>
>
> Is this not the behaviour of require.js? It is (I think) intended for the
> case where code is converted from a commonjs module. It is first scanned for
> sync require calls (var a = require('a')), which are pulled up to the define
> call level and a local require is passed in, effectively preloaded with a.
> Can see that this could be fragile if used for other use cases though.
>
>
> If that is a behaviour of requirejs I've never used it, nor would I. I
> prefer to have my code rewritten by the maintainer if it needs to be
> rewritten.
>

Yeah, I'd like this to happen ahead of time (at build time perhaps) for
modules I author and maintain in commonjs module format, but would like to
provide as AMD modules.


> But it wasn't clear at all from *your* code that you intended all your
> "syncRequire" calls to be rewritten into simple requires. How would that
> happen exactly? Do you intend to implement that with actual code someday?
> How will such code handle more complicated (but perfectly valid) syntax
> involving calls to your syncRequire inside a nested scope?
>
> define('x', { flip: 1 });
> define('y', function() {
>     var x = { flip: 0; }
>     (function(){
>         var a,
>             x = y = { flip: 1? syncRequire('x').flip : 0 };
>         if (x.flip !== 1) throw 'fail';
>     })();
>     if (x.flip !== 0) throw 'fail';
> });
>
> It seems like it would be much more reliable for the author to simply
> remove the syncRequire and write something like this instead:
>
> define('x', { flip: 1 });
> define('y', ['x'], function(myX) {
>     var x = { flip: 0; }
>     (functi on(){
>         var a,
>             x = y = { flip: 1? myX.flip : 0 };
>         if (x.flip !== 1) throw 'fail';
>     })();
>     if (x.flip !== 0) throw 'fail';
> });
>
> I'd be very interested in seeing how you would *automate* something like
> that though. Certainly doing so *correctly* and staying "tiny" would be a
> challenge, in my estimation.
>
> On the other hand, if you intended syncRequire to simply be a way to access
> an already loaded module, as I originally assumed, then it is either
> unnecessary or unreliable. In the case where the module is listed as a
> dependency (as in my second example above) it is unnecessary since the
> module is already guaranteed to be there and can be listed as one of the
> formal parameters to your builder function; In the case where it is not
> listed as a requirement (as in my first example) then it is not guaranteed
> to be there and so should be expected to *sometimes* be unavailable to
> use.
>

Sync require is only ever exposed as a "require" dependency and parameter to
the factory function (it is private to its enclosing scope I think):

define('b', ['require', 'a'], function (require) {
    var a = require('a');
});

If you omit the dependencies, it's supposed to default to ['require',
'exports', 'module'] (this needs to be fixed in require-tiny), so you can
write:

define('b', function (require) {
    var a = require('a');
});

I think this case should fail as "a" is not declared up-front. With the
current implementation in require-tiny, it may or may not work depending if
it has previously been loaded, which doesn't seem desirable, so I think I'll
change it to only work for explicit dependencies. The spec doesn't make this
bit entirely clear:

http://wiki.commonjs.org/wiki/Modules/AsynchronousDefinition

>
>
> Secondly there doesn't appear to be any code to deal with the case where
> the a module is async required, and so requested via HTTP, and then required
> again before that first HTTP request returns.
>
>
> Good spot, will add this after I fix the code.
>
>
> gloader had the ability to track the lifecycle of a require/request, and
> could see this condition existed and therefore refrain from making the
> second HTTP r equest. tiny seems to just make two requests(?) and carry on.
> This all gets even more complicated if you add the case of an async require
> followed by (but before the async returns) a sync require. gloader had to
> deal with this case too. If I were being Agile I'd suggest you ditch
> the syncRequire for the first release; it's a very hard thing to get right
> when it is combined with async requires.
>
>
> The sync require (perhaps misnamed) doesn't actually do any loading, it's
> just there for the use case described above. I don't intend to do any actual
> synchronous loading as it's not required by AMD and doesn't seem practical -
> this was the same conclusion you guys came to with glow2 right?
>
>
> Yes, that was the reason we ditched sync loading when it came time to
> rewrite gloader. And it's exactly why I'm suggesting you ditch syncRequire
> from your own proposal now. For many reasons it is a raft of problems,
> without much (if any) benefit.
>

Like I said, isnt actually doing any synchronous loading, is just returning
already loaded modules.

Tom


>
>
> Tom
>
>
>
> --
> Michael Mathews
>
> On Thursday, 5 May 2011 at 23:36, Tom Yandell wrote:
>
> Not node.js related but anyway...
>
> Michael: want to code review this for me?
>
> https://github.com/tomyan/require-tiny/blob/master/src/require-tiny.js
>
> No tests yet - is a dirty prototype :-) Is 2697 bytes, 933 bytes minified
> and 568 bytes gzipped.
>
> Tom
>
>
>
>
>

Re: [nug] code review request - require-tiny

From:
Michael Mathews
Date:
2011-05-07 @ 13:22
On Saturday, 7 May 2011 at 13:54, Tom Yandell wrote:
define('b', ['require', 'a'], function (require) {
>  var a = require('a');
> });
> 
 ^ I can only repeat that this seems unnecessary to me, since I could more
easily omit the sugar and write: 

define('b', ['a'], function (a) {
// just use 'a' here
});


> define('b', function (require) {
>  var a = require('a');
> });

^ And I agree this should fail, but unfortunately it could succeed or fail
in apparently random ways, depending on how things are evaluated in other 
parts of the code.

I see no benefit to having in inline require if I am writing the code as 
an AMD myself, and you haven't yet explained how you would implement a 
build script to reliably and automatically convert a commonjs module into 
an AMD one whilst avoiding the scoping fails I pointed out previously -- I
would enjoy coming up with difficult test cases for such a script. 
Regardless, assuming you could do that, such a build script would be 
completely separate to any code inside the require-tiny.js file you asked 
me to review, and I still say that your syncRequire function should go.



Re: [nug] code review request - require-tiny

From:
Michael Mathews
Date:
2011-05-07 @ 13:27
Ah, forget explaining how to write the build script, I already see a way 
to do it. :)

-- 
Michael Mathews
On Saturday, 7 May 2011 at 14:22, Michael Mathews wrote: 
> On Saturday, 7 May 2011 at 13:54, Tom Yandell wrote:
> > define('b', ['require', 'a'], function (require) {
> >  var a = require('a');
> > });
> > 
> 
>  ^ I can only repeat that this seems unnecessary to me, since I could 
more easily omit the sugar and write: 
> 
> define('b', ['a'], function (a) {
> // just use 'a' here
> });
> 
> 
> > define('b', function (require) {
> >  var a = require('a');
> > });
> > 
> 
> ^ And I agree this should fail, but unfortunately it could succeed or 
fail in apparently random ways, depending on how things are evaluated in 
other parts of the code.
> 
> I see no benefit to having in inline require if I am writing the code as
an AMD myself, and you haven't yet explained how you would implement a 
build script to reliably and automatically convert a commonjs module into 
an AMD one whilst avoiding the scoping fails I pointed out previously -- I
would enjoy coming up with difficult test cases for such a script. 
Regardless, assuming you could do that, such a build script would be 
completely separate to any code inside the require-tiny.js file you asked 
me to review, and I still say that your syncRequire function should go. 

code review request - require-tiny

From:
Tom Yandell
Date:
2011-05-05 @ 22:37
Not node.js related but anyway...

Michael: want to code review this for me?

https://github.com/tomyan/require-tiny/blob/master/src/require-tiny.js

No tests yet - is a dirty prototype :-) Is 2697 bytes, 933 bytes minified
and 568 bytes gzipped.

Tom

Re: [nug] code review request - require-tiny

From:
Michael
Date:
2011-05-05 @ 22:52
Congratulations. You are entirely predictable.

On 5 May 2011, at 23:37, Tom Yandell <tom@yandell.me.uk> wrote:

> Not node.js related but anyway...
> 
> Michael: want to code review this for me?
> 
> https://github.com/tomyan/require-tiny/blob/master/src/require-tiny.js
> 
> No tests yet - is a dirty prototype :-) Is 2697 bytes, 933 bytes 
minified and 568 bytes gzipped. 
> 
> Tom

Re: [nug] code review request - require-tiny

From:
Michael
Date:
2011-05-05 @ 22:55
By the way, is this like perl golf. Do I get points for removing bytes?

On 5 May 2011, at 23:37, Tom Yandell <tom@yandell.me.uk> wrote:

> Not node.js related but anyway...
> 
> Michael: want to code review this for me?
> 
> https://github.com/tomyan/require-tiny/blob/master/src/require-tiny.js
> 
> No tests yet - is a dirty prototype :-) Is 2697 bytes, 933 bytes 
minified and 568 bytes gzipped. 
> 
> Tom

Re: [nug] code review request - require-tiny

From:
Tom Yandell
Date:
2011-05-05 @ 23:05
I know, I couldn't help it. And yes you do (using uglify to minify).

Tom

On 5 May 2011 23:55, Michael <micmath@gmail.com> wrote:

> By the way, is this like perl golf. Do I get points for removing bytes?
>
>
> On 5 May 2011, at 23:37, Tom Yandell <tom@yandell.me.uk> wrote:
>
> Not node.js related but anyway...
>
> Michael: want to code review this for me?
>
> <https://github.com/tomyan/require-tiny/blob/master/src/require-tiny.js>
> https://github.com/tomyan/require-tiny/blob/master/src/require-tiny.js
>
> No tests yet - is a dirty prototype :-) Is 2697 bytes, 933 bytes minified
> and 568 bytes gzipped.
>
> Tom
>
>

code review request - require-tiny

From:
Tom Yandell
Date:
2011-05-05 @ 22:36
Not node.js related but anyway...

Michael: want to code review this for me?

https://github.com/tomyan/require-tiny/blob/master/src/require-tiny.js

No tests yet - is a dirty prototype :-) Is 2697 bytes, 933 bytes minified
and 568 bytes gzipped.

Tom

code review request - require-tiny

From:
Tom Yandell
Date:
2011-05-05 @ 22:36
Not node.js related but anyway...

Michael: want to code review this for me?

https://github.com/tomyan/require-tiny/blob/master/src/require-tiny.js

No tests yet - is a dirty prototype :-) Is 2697 bytes, 933 bytes minified
and 568 bytes gzipped.

Tom