Added factory support for node.js #300

Merged
merged 7 commits into from Feb 16, 2013

7 participants

@tommymessbauer

Adds a create() method to the Handlebars object which will generate a new instance of Handlebars in node.js. The npm test is passing. I am not sure how the browser version is tested. Please let me know if there are any issues or feedback.

I really love the interfaces and DI hooks in handlebars. We are moving to it for our main rendering engine. Our company hosts a couple hundred sites and the singleton pattern is blocking our migration. Ideally, I would love to have this merged to core and published to npm sooner rather than later. If this is not possible, then we'll run from our fork in the near term.

I am here to help with anything. Please le me know if there is anything I can do to help.

Tommy Messbauer added some commits Aug 29, 2012
@travisbot

This pull request passes (merged eccc7c3 into 89f5ab8).

@kpdecker
Collaborator

We should drop the tab indent at minimum for all of the content in between the BROWSER comments as this is going to impact non-node users (this adds a slight cost for node comprehension but if the dev is familiar with the BROWSER parsing that is done on the files it shouldn't be a concern).

After that there are a few places that I think need to be looked at for impact in browser land but can't do line-by-line comments on ?w=1 mode so will come back after the tabs are dropped back.

@Calamari

The tab indendation in aprox. all files confuses what the real changes are.

@tommymessbauer

I'll spend some time to remove the extra tabs so that it is easier to see what changed.

I also just re-read some of the code and I am pretty sure that I broke something on the client side package. I need to figure out how to run the client side unit tests.

@bwindels

I second this request. This is indispensable to avoid race conditions if you have helpers that need data specific to the request.

@wycats
Owner

@tommydudebreaux any progress here?

@tommymessbauer

Sorry for going dark. My fork has been sufficient for delivering some projects. I can update the formatting. Just so I understand, we want 2 spaces and no tabs. Yes?

@tommymessbauer

FWIW- I published this to npm so we could move forward with our other work a while back.

https://npmjs.org/package/tommydudebreaux-handlebars

I am going update the formatting this weekend

@kpdecker
Collaborator

@tommydudebreaux I think on my initial review https://github.com/wycats/handlebars.js/pull/300/files?w=1#L1L1 seemed suspect WRT the client-side changes.

The rake tests are going to be the closest to the the client environment and the npm test tests are going to be representative of the node environment.

2 spaces, no tabs. And I'd argue for not indenting the BROWSER sections of code to match the node logic as this makes for weird output in the non-minimized client code, more diff changes here, and generally I don't think we should be going for node only features when implementing things here so the amount of node specific code should hopefully be small.

@tommymessbauer

Man.. I finally got a second to update this today. In addition to spacing, I pulled latest upstream/master, merged it, and insured the tests are still passing.

-T

@kpdecker kpdecker commented on the diff Dec 2, 2012
lib/handlebars/base.js
-(function(Handlebars) {
@kpdecker
Collaborator
kpdecker added a line comment Dec 2, 2012

This causes a few module variables to be promoted to globals in the browser version. We need to make sure that we maintain this module scope.

@kpdecker
Collaborator
kpdecker added a line comment Feb 16, 2013

This is still an open issue. These vars will not be exposed as globals where they were previously not:

var toString = Object.prototype.toString, functionType = "[object Function]";
@kpdecker
Collaborator
kpdecker added a line comment Feb 16, 2013

I'm looking into this now as I need to push a release for rc3.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@kpdecker kpdecker and 1 other commented on an outdated diff Dec 2, 2012
lib/handlebars/compiler/ast.js
// BEGIN(BROWSER)
-(function() {
@kpdecker
Collaborator
kpdecker added a line comment Dec 2, 2012

nit: Can we maintain the prior formatting here?

@tommymessbauer
tommymessbauer added a line comment Jan 15, 2013

So you just want this section un-indented?

@kpdecker
Collaborator
kpdecker added a line comment Jan 15, 2013

Two options here. Restore the function scope so the verifyMatch vare is not escaped as a global or inline it in the BlockNode instance. For the sake of clarity on this specific PR the former might be better.

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

What needs to be done to get this guy merged?

@tommymessbauer

Can we get together a list of things that need to happen to merge this?

It is stable on my end and we have been running it in production with client and server rendering for months via these published repos. I would love to have this as part of the core.

https://github.com/tommydudebreaux/handlebars.js
https://npmjs.org/package/tommydudebreaux-handlebars

@kpdecker
Collaborator

I think my only concerns right now are changes to the scopes on the client side that it seems like this introduced. If we can restore the scoping on the base and ast files in the client portion of the code I think we will be in a good place.

Tommy Messbauer added some commits Feb 12, 2013
Tommy Messbauer restored scope of var verifyMatch 514c939
Tommy Messbauer merge 1ca7462
Tommy Messbauer tests passing 7f9e3fe
@tommymessbauer

Sorry for delay on knocking this out. Been super busy with work and new baby... Anyways, so the verifyMatch function is inlined now. I also merge upstream and ensured tests pass.

Crossing fingers we are good now. :)

@wshaver wshaver commented on the diff Feb 15, 2013
dist/handlebars.js
@@ -24,10 +24,7 @@ THE SOFTWARE.
// lib/handlebars/base.js
-/*jshint eqnull:true*/
-this.Handlebars = {};
-
-(function(Handlebars) {
+var Handlebars = {};
@wshaver
wshaver added a line comment Feb 15, 2013

Yes please

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@kpdecker kpdecker merged commit 7f9e3fe into wycats:master Feb 16, 2013

1 check passed

Details default The Travis build passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment