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

How to pass a different yaml parser #500

Closed
1 task done
Kikobeats opened this issue Aug 31, 2018 · 11 comments
Closed
1 task done

How to pass a different yaml parser #500

Kikobeats opened this issue Aug 31, 2018 · 11 comments
Labels
3.0 Candidate for breaking change in 3.0 release pr-welcome

Comments

@Kikobeats
Copy link

Kikobeats commented Aug 31, 2018

I'm submitting a ...

  • feature request

What is the current behavior?

Currently, the library is forcing to use js-yaml or yaml dependency as parser, but you can't a different dependency, for example, yaml-parser, that in my opinion is better and smaller.

Also, the way for loading these dependencies is making the code so fragile.

https://github.com/lorenwest/node-config/blob/master/lib/config.js#L882

These libraries are not part of the dependencies projects, so it doesn't make sense force the user choose between them.

I suggest a simple, open and more clean way: just expose an option to be possible specify the yaml parser that you want to use:

{
	yamlParser: require('yaml-parser').safeLoad
} 
@lorenwest
Copy link
Collaborator

The parser used must be defined before files are parsed so they can't be specified in config files.

I suggest creating environment variables that define parser libraries to use for different file types. Something like this:

export NODE_CONFIG_PARSER_YAML=yaml-parser

These environment variables could override the defaults, and even let you define parsers for your own file format

export NODE_CONFIG_PARSER_FUNKY=funky-parser

That would look for {filename}.funky files, and parse them. These parsers would have to have a single method parse(text) that returns a parsed JS object from the text.

@lorenwest
Copy link
Collaborator

This would be a nice addition to the config library - fitting for issue #500. Any takers?

@Kikobeats
Copy link
Author

@lorenwest notes that your solution only works if the parser loaded is the main function exported.

yaml-parser.safeLoad doesn't fit into a environment variable, or you need to provide another environment variable for the method.

@lorenwest
Copy link
Collaborator

lorenwest commented Sep 1, 2018

The environment variable could reference a module in your local app that wraps the parser for custom loading, and would export a parse(text) method that forwards the call with custom arguments, etc.

export NODE_CONFIG_PARSER_FUNKY=./lib/funkyParserWrapper

@markstos
Copy link
Collaborator

markstos commented Sep 4, 2018

Does it really matter if the YAML parser is optimal? It's used once when the first second of the app starting up, and then not again after that.

If you are concerned about the small extra RAM taking up by the YAML parser after it is used, it can be unloaded from memory.

Unloading parsers after we are done with them is a small memory optimization that node-config could make-- but it's only optimization if the module is never used by the rest of the app, so it's perhaps best left up to the user.

I don't think there's a strong case to be made for alternate parsers to improve either performance or memory usage. However, if a case can be made that an alternate parser is more correct, that's interesting.

@Kikobeats
Copy link
Author

@markstos probably doesn't matter too much, but then another point to consider: Why no remote the try/catch workflow to load the right dependency and just use one?

@markstos
Copy link
Collaborator

markstos commented Sep 5, 2018

@Kikobeats Six years ago, we likely didn't need to add support for a second YAML parser in the first place. Now that we support two, removing support for either one would break some code, as neither YAML parser is a hard dependency. We could make such a simplification as part of a major-version upgrade where we breakwards compatibility. It's a reasonable suggestion to simplify.

@Kikobeats
Copy link
Author

Yes, totally reasonable ship that as breaking change

@markstos markstos added 3.0 Candidate for breaking change in 3.0 release and removed pr-welcome labels Sep 5, 2018
@markstos
Copy link
Collaborator

markstos commented Sep 5, 2018

I've added the 3.0 label for this to track it.

@iMoses
Copy link

iMoses commented Jun 27, 2019

This is can be solved by addressing #181
Simply override a parser instead of defining a new file type.

@markstos
Copy link
Collaborator

Closing in favor of #181.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.0 Candidate for breaking change in 3.0 release pr-welcome
Projects
None yet
Development

No branches or pull requests

4 participants