Adding NodeJS support + Synchronous API (issues 7 & 43) #79

Closed
wants to merge 12 commits into
from

6 participants

@nopnop
  • Adding a package.json (fix issue 7)
  • Rainbow.color() return result synchronously (fix issue 43)
  • language are required automatically
  • Adding a small demos and some documentation in README.md
@twuni

Do we still need to do this? This should be taken care of in line 22.

@twuni

Logically, I would expect the contextual this to receive the Rainbow object, whatever it happens to be. This is slightly different than var Rainbow, which could result in dereferencing the Rainbow object when the scope ends, and which will not export Rainbow to the this object.

@twuni

window should be replaced with this and these checks need to be more rigorous:

if( typeof(this.addEventListener) === "function" ) {
    this.addEventListener( "load", Rainbow.color, false );
} else if( typeof(this.attachEvent) === "function" ) {
    this.attachEvent( "onload", Rainbow.color );
}
@twuni

This should not be mutually exclusive with attaching DOM events, because it's conceptually unrelated. Scratch the else.

if( module && module.exports ) {
    module = module.exports = Rainbow;
}
@twuni

Drop the reference to window. We only care about whether this supports DOM events, and we know that this will be defined.

@twuni

Try this technique instead for overriding a function to avoid scope pollution:

// Assuming Foo.bar = function( baz ) { ... }
Foo.bar = ( function( _super ) {
    return function( baz ) {
        // ...your code here...
        return _super.apply( this, arguments );
    };
} )( Foo.bar );
@twuni

Should wrap this line in a sanity check:

if( typeof(require) === "function" ) {
    require( "./js/language/" + language + ".js" );
}
@twuni

Why did you change this? I think it was more readable before, and presumably would have worked equally well in a browser with require.js.

@twuni

Why did you remove this block? It was awesome, and cleaner than yet functionally equivalent to some of the inline patching you've done in rainbow.js for automatic language loading.

@twuni

Why not override setTimeout just for this file if running within a Node context? Up at the top somewhere...

if( isNode && typeof(setTimeout) === "function" ) {
    setTimeout = function(f) { return f(); };
}

Something like that would prevent the need for this conditional, and would not affect setTimeout outside of this file.

@twuni

Another instance where a contextual override of setTimeout would be beneficial, rendering this conditional unnecessary.

@twuni

I'm wondering why you moved this block of code from an overload in index.js to an inline logic branch within the color function itself. It just seems messier this way, having isNode conditionals sprinkled throughout.

@nopnop

Thank you for this complete and full of common sense review. Some comments and corrections coming.

@nopnop

I removed all specific nodejs code inside languages files (and using require like this is not compatible with requirejs)

@nopnop

I removed all the specific node test : setTimeout is now locally override when using nodejs

@nopnop

Preventing error throwing when calling this method without callback : usually the case when using nodejs

@nopnop

Inspired from underscore.js to handle global scope and exporting nodejs module

@nopnop

I currently write a clean test suite as there is some bugs using node. (Browser mode is ok, even with minified version)

@nopnop

No need to check for 'require' method, as this script is only for server side.

@nopnop

All is ok now : the new test suite for node (tests/tests-node.js) and the original test suite for the browser return the same results.

Some review ?

@ericf

@ccampbell Is there a plan to merge this in?

@EliSnow

I too would like to see this implemented.

@KenanY KenanY and 1 other commented on an outdated diff May 29, 2013
@@ -0,0 +1,9 @@
+{ "name": "rainbow"
+ , "description": "A code syntax library written in Javascript"
@KenanY
KenanY added a line comment May 29, 2013

There are two spaces between "syntax" and "library" here. Also, I don't think "code syntax library" properly describes Rainbow.

@KenanY
KenanY added a line comment May 29, 2013

A .repository field is also needed to prevent npm from bugging users about this package missing it.

@nopnop
nopnop added a line comment May 29, 2013

The description field come from the readme.

@KenanY
KenanY added a line comment May 29, 2013

Yeah but the word "highlighting" is missing:

Rainbow is a code syntax highlighting library written in Javascript.

@nopnop
nopnop added a line comment May 29, 2013

Whoops. Thanks ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@caridy

can we get this pkg in npm?

@ccampbell
Owner

Believe it or not, even though this project has not gotten much love recently, I actually am working on version 2.0 which will work in Node out of the box. Hopefully I will have it ready within the next couple weeks.

@caridy

awesome, for now we are just locking down the dep to the current master SHA:

  "devDependencies": {
    "Rainbow": "ccampbell/rainbow#c2f5b0a3fa"
  }

looking forward to try out v2.

@ccampbell
Owner

Support for node.js and Rainbow.colorSync was added in version 2.0 which finally was released.

npm install rainbow-code
var rainbow = require('rainbow-code');
var highlightedCode = rainbow.colorSync('var finally = true;', 'javascript');
@ccampbell ccampbell closed this Jul 2, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment