Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

wish: make config file format support pluggable. #181

Closed
markstos opened this issue Dec 2, 2014 · 18 comments
Closed

wish: make config file format support pluggable. #181

markstos opened this issue Dec 2, 2014 · 18 comments
Assignees

Comments

@markstos
Copy link
Collaborator

markstos commented Dec 2, 2014

Supporting additional file formats should be easy.

The first step for this task is to decide on the design to use.

Possible designs

  • node-rechoir handles this problem and could be added as dependency, effectively outsourcing the problem. This is what gulp uses. node-config currently has no dependencies, so this would be a change in this regard.
  • TODO: Add more possibilities here
@lorenwest
Copy link
Collaborator

To expand a bit, I'd like to see pluggable config sources. Node-config only supports file type config sources, and if we generalize config sources to include asynchronous sources, the plugins could be written for different file extensions, http sources, db sources, etc.

Supporting async sources will be a little work, but will remove the biggest restriction with node-config - that it's file based only.

@markstos
Copy link
Collaborator Author

markstos commented Dec 9, 2014

My recommendation is outsource the file format support to node-rechoir. I think that can be done transparently and be backwards compatible, but does mean adding a dependency. I think async-source support may be best considered separately. I've opened #186 to consider options there.

@lorenwest
Copy link
Collaborator

Good plan. Do you think this refactor can be added into the 1.x code stream? If it doesn't introduce incompatibilities, then we don't have to wait until the 2.x release.

@markstos
Copy link
Collaborator Author

markstos commented Dec 9, 2014

It can be tried on a branch and we'll find out.

There are perhaps two levels here, one that could be now and the other later:

  • Initially, we could simply improve file format support
    • As a second step, we could allow customer support, such that one might be able to express: { 'coffee' : 'iced-coffee' }.

That is: You could declare your own compilers or loaders for any extension, including ones that already have a default loader. I haven't worked out the details of the interface for that yet.

@markstos
Copy link
Collaborator Author

@lorenwest, currently we load the various file formats we support in a specific order:

 var extNames = ['js', 'json', 'json5', 'coffee', 'iced', 'yaml', 'yml', 'cson', 'properties'];   

...but we don't document what this order is and it should therefore not be depended on. In our common use-case, a file has only one of the extensions and no siblings with the exact same name but a different extension, so the load order doesn't matter anyway.

As we move to making the file format support, it would be ideal to remove this hardcoded ordered list of extensions from our source code.

Would you be OK with moving to trying the extensions we support in alphabetical order?

Given that the order doesn't matter for the common use-case, I think this is a reasonable trade-off for the flexibility.

Alphabetical order should remain fairly predictable if it ever happens to matter. While new extensions may be added later, the alphabetical order between any two pre-existing extensions would remain unchanged.

@markstos
Copy link
Collaborator Author

I looked more tonight at possibly using the interpret module for this. One edge case for this is that we strip comments out of a couple file formats. Those cases we can continue to handle internally for now.

I also found that interpret does not currently understand the .properties or .json5 extension, so I opened "wish" requests for each of those to be supported so we can outsource loading those formats.

https://github.com/tkellen/node-interpret/issues

@lorenwest
Copy link
Collaborator

Having a consistent and determinant load order between extensions makes sense. That change would be part of a 2.x release because it could unwittingly break existing installs.

It appears that interpret/rechoir aren't libraries that do the loading. Interpret is a database of file extensions to interpreters, and rechoir is a library that load the interpreters so the files can be loaded using require().

Most installations wouldn't need more than one or two file formats. It'd be nice to have a bunch of config modules config-json5, config-iced-coffee, etc. available to choose from. Any one install could require one of these modules before requiring config. That's a plug-in methodology vs. an all-inclusive methodology like rechoir and the current node-config.

With a plugin protocol we could offer comment filtering as an option, and plugin authors wouldn't need permission to write and publish a parser for their favorite syntax.

@markstos markstos mentioned this issue Jan 4, 2015
@vicary
Copy link
Contributor

vicary commented Jul 11, 2015

Came here when looking for the extension priority.

If you are suggesting users to not create more than one file ext. under the same name, I suggest putting this in the document.

@tamtakoe
Copy link

tamtakoe commented Sep 4, 2015

config-parser module may have the following structure:

config-parser
|- index.js
|- plugins
   |- json
   |- yaml
   |- cson
   |- ...
var configParser = require('config-parser');

//Register custom plugin
configParser.registerPlugin(['toml', 'tml'], function(input) { ... return output });

var config = configParser.parse(file); //string/buffer/vinyl

@alexindigo
Copy link

@markstos I gave it a try in a separate library (to allow myself to start with a clean slate and try different approaches), and I came up with something that should be powerful and flexible enough for different use cases.

This how it looks:

var configly = require('configly');

configly.PARSERS = {
  ini       : ini.parse,
  // have it as a wrapper to prevent extra arguments leaking 
  cson      : function(str) { return cson.parse(str); },
  yml       : function(str) { return yaml.safeLoad(str); },
  // same options as used within `config` module 
  properties: function(str) { return properties.parse(str, {namespaces: true, variables: true, sections: true}); },
  // use json5 instead of `JSON.parse` 
  json      : json5.parse
  // keep the original one 
  js        : configly.PARSERS.js,
};

var configObj = configly();

It is still a singleton, but the module exports load function,
instead of single config object, so it allows more flexibility,
for the price of extra function call.

@ltvan
Copy link

ltvan commented Jul 20, 2017

I see the suggestion of tamtakoe looks good, simple and easy. Why don't just go with it?

Currently I have an issue that I want to have a custom type with js-yaml, but since you have hard-coded the .load call so I cannot inject my custom type. I tried to wrap it with global.Yaml but you declared var Yaml at the top, then the global variable can not be reached.

@markstos
Copy link
Collaborator Author

markstos commented Dec 29, 2017

@lorenwest See the proposal for pluggable config parsing above. Also note that it came up again a couple of hours ago when someone wanted to use a different algorithm to parse YAML.

Here's a related design option.

config/parsers.js

Check for a new config file, config/parsers.js. If present, it is expected to return a JavaScript object that maps file extensions to functions which parse the file extension.

The presence if of a known extension there will override the default parsers and the presence of unknown extensions will allow new ones to parsed without support in the core.

// config/parsers.js

module.exports = {
  ini       : ini.parse,
  // have it as a wrapper to prevent extra arguments leaking 
  cson      : function(str) { return cson.parse(str); },
  yml       : function(str) { return yaml.safeLoad(str); },
  // same options as used within `config` module 
  properties: function(str) { return properties.parse(str, {namespaces: true, variables: true, sections: true}); },
  // use json5 instead of `JSON.parse` 
  json      : json5.parse
};

Alternatives considered

  • Previously we looked at outsourcing config handling to either rechoir or node-interpret, but we ran into complications including: different extension preference order, different file formats supported, or variation in how particular file formats were supported (like stripping comments in JSON). They also require the additional dependency and some amount of coordination between projects. All the downsides would be addressed by an in-house solution.
  • @tamtakoe earlier in this thread proposed a seperate config-parser module, seen above, but seems perhaps overly complicated compared to the simple-object solution proposed by @alexindigo.

Using another simple configuration file with object overrides seems to fit very naturally into how the rest of node-config works. It also allows you to configure the parsers in a single place and not have any code in every module where you load node-config.

@lorenwest
Copy link
Collaborator

This is a perfect balance of simplicity and flexibility. I support this change.

@alexindigo
Copy link

Great idea @markstos

@markstos
Copy link
Collaborator Author

Thanks for the prompt feedback. I'm adding the "PR Welcome" tag. Perhaps one of the issue followers here or @stalniy from a related ticket will be interested in looking at it.

@iMoses
Copy link

iMoses commented Jun 27, 2019

@markstos I like the proposal, except that instead of using a convention (predefined filename) I suggest we make it into an env-var pointing to the parser's extension (relative or absolute).

It's a pretty straightforward solution, I can implement it if it's a priority.

@iMoses
Copy link

iMoses commented Jun 29, 2019

@markstos solved by #557, can be closed

@markstos
Copy link
Collaborator Author

@iMoses Thanks again for implementing a feature I wanted several years ago. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants