hapipal / confidence

Dynamic, declarative configurations
Other
264 stars 44 forks source link

RangeError with Handlebars #29

Closed gergoerdosi closed 9 years ago

gergoerdosi commented 10 years ago

I have this configuration:

var Confidence = require('confidence');
var Handlebars = require('handlebars');

var store = new Confidence.Store();

var config = {
  views: {
    engines: {
      html: {
        module: Handlebars
      }
    }
  }
};

store.load(config);
module.exports = store.get('/', { env: process.env.NODE_ENV });

Which results in an error:

/example/node_modules/confidence/lib/store.js:225
    var keys = Object.keys(node);
                      ^
RangeError: Maximum call stack size exceeded

It is happening because Store.validate tries to inspect the Handlebars module which probably has a self reference. How to handle this issue?

gergoerdosi commented 10 years ago

Realized that objects can't be used as values:

Values can contain any non-object value (e.g. strings, numbers, booleans) as well as arrays.

Submitted a PR which supports objects as values.

patrickkettner commented 10 years ago

Hey there, @gergoerdosi Thanks a ton for submitting the PR for the issue!

I am confused as to why you would be loading the config in such a way. Why is the views section of your hapi server in your confidence file?

gergoerdosi commented 10 years ago

It's a plugin's configuration. Part of the config file is environment specific, that's why I use confidence and its $filter: 'env'. I use a workaround now:

var Confidence = require('confidence');
var Handlebars = require('handlebars');

var store = new Confidence.Store();

var config = {
    transport: {
        $filter: 'env',
        production: {
            ...
        },
        development: {
            ...
        }
    }
};

store.load(config);

module.exports = (function () {

    var config = store.get('/', { env: process.env.NODE_ENV });

    config.views = {
        engines: {
              html: {
                  module: Handlebars
              }
          }
    };

    return config;
})();

However it's not the nicest solution. That's why I thought about adding $parse: false to confidence, so the view property can be part of the confidence object. I'm open to other solutions though. Please let me know if you have a better idea.

gergoerdosi commented 10 years ago

@patrickkettner Any further feedback on this?

patrickkettner commented 9 years ago

Hey @gergoerdosi! So sorry I took so long to reply.

Unfortunately, however, I still don't understand what it is you are doing, really (or rather why you are going about it this way).

What is the plugin? Could I check out how that works? I am really hesitant to add this because it seems like the solution to an xy problem, but I would love to see what you are doing that requires this fix (and I promise I will reply right off this time :] ).

gergoerdosi commented 9 years ago

This problem is not unique to plugins. Here are two examples:

First example: I store the configuration of the server in a separate file which uses confidence. This allows me to set different values for different environments (host, port, etc).

settings.js:

var Confidence = require('confidence');
var Handlebars = require('handlebars');

var store = new Confidence.Store();

var settings = {
    connection: {
        host: '127.0.0.1',
        port: {
            $filter: 'env',
            production: 80,
            $default: 8000
        },
    },
    views: {
        engines: {
            html: {
                module: Handlebars.create(),            // <- causes an error
                relativeTo: Path.join(__dirname, 'lib/templates'),
                partialsPath: 'partials',
                helpersPath: 'helpers'
            }
        },    
    }
};

store.load(settings);
module.exports = store.get('/', { env: process.env.NODE_ENV });

server.js:

var Hapi = require('hapi');
var settings = require('./settings');

var server = new Hapi.Server();
server.connection(settings.connection);
server.views(settings.views);

The reason I store settings in a separate configuration file is because I have a different server.js file for the test environment. The configuration file allows me to store the configuration in one place, but load from multiple places.

Second example: I use a similar configuration for the hapi-mailer plugin (https://github.com/gergoerdosi/hapi-mailer).

settings/hapi_mailer.js:

var Confidence = require('confidence');
var Handlebars = require('handlebars');

var store = new Confidence.Store();

var settings = {
    transport: {
        $filter: 'env',
        production: {
            host: 'example.com',
            port: 2525,
            auth: {
                user: 'user',
                pass: 'password'
            }
        },
        test: {
            host: 'example.org',
            port: 2525,
            auth: {
                user: 'user',
                pass: 'password'
            }
        }    
    },
    views: {
        engines: {
            html: {
                module: Handlebars.create(),            // <- causes an error 
                layoutPath: 'templates/emails/layouts',
                layout: 'default'
            },
            text: {
                module: Handlebars.create()            // <- causes an error
            }
        },
        relativeTo: Path.join(__dirname, 'lib/templates/emails')
    }
};

store.load(settings);
module.exports = store.get('/', { env: process.env.NODE_ENV });

server.js:

var plugin = {
    register: require('hapi-mailer'),
    options: require('./settings/hapi_mailer')
}

server.register(plugin, function (err) {
    ...
});

Not sure these examples work (they are just similar to the real code), but I think you get the idea.

gergoerdosi commented 9 years ago

@patrickkettner Do you need more information? I really would like this issue to be resolved somehow.

arb commented 9 years ago

Do you use a different view engine depending on your environment? That would be extremely unusual.

gergoerdosi commented 9 years ago

@arb No, I don't. However part of the configuration is dependent on the environment, for example host and port. See the hapi-mailer example.

arb commented 9 years ago

Can't you remove the entire views section and just include that in your server bootstrap file rather than in a confidence build?

gergoerdosi commented 9 years ago

@arb Not really. This is a plugin's own view and the register interface only accepts an options object:

var plugin = {
    register: require('hapi-mailer'),
    options: require('./settings/hapi_mailer')
};

server.register(plugin, function (err) {
    ...
});

I could do something like this:

var options = require('./settings/hapi_mailer');
options.views = {
    engines: {
        ...
    }
};

var plugin = {
    register: require('hapi-mailer'),
    options: options
};

server.register(plugin, function (err) {
    ...
});

But don't think it's nicer. I can workaround the problem in many ways. It would be just good if confidence supported objects too. If you don't want to add this change to confidence, fine, just please tell me then, so I can find an other solution.

patrickkettner commented 9 years ago

@gergoerdosi I really appreciate your patience, but I just don't think this is something that should be added to confidence.