-
Notifications
You must be signed in to change notification settings - Fork 776
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
feat: add appender and level inheritance #863
feat: add appender and level inheritance #863
Conversation
feat: add appenders and level inheritance feat: inherit appenders and level from base feat: inherit appenders, handle empty categories
…harapiak/log4js-node into inherit-parent-category-config
Codecov Report
@@ Coverage Diff @@
## master #863 +/- ##
==========================================
+ Coverage 97.43% 97.49% +0.05%
==========================================
Files 25 25
Lines 937 959 +22
==========================================
+ Hits 913 935 +22
Misses 24 24
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #863 +/- ##
==========================================
+ Coverage 97.86% 97.92% +0.06%
==========================================
Files 25 25
Lines 937 966 +29
==========================================
+ Hits 917 946 +29
Misses 20 20
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to work out how to support your "pre-processing" of the config object before it gets passed to the other modules in a nice way. Right now it will break when read-only config is used, and it goes against the way I've tried to keep the main module from having to know about all the config-related stuff. I'd suggest adding an extra hook into the configuration module for adding pre-processing functions. What do you think?
lib/log4js.js
Outdated
@@ -59,6 +59,8 @@ function configure(configurationFileOrObject) { | |||
} | |||
debug(`Configuration is ${configObject}`); | |||
|
|||
categories.addInheritedConfig(configObject); // copy config from parent to child categories |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is going to give you problems when users supply read-only config objects (i.e. they've used Object.freeze
or something similar). I think the node-config
package does this by default. Your addInheritedConfig
function modifies the original configObject. That's why the other modules add themselves as config listeners (there's code in the categories module that does this too) - they'll get called after the config object has been cloned, like below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I wasn't aware of read only config objects.
I found that I had to put some inheritance 'fixes' into the configuration validation, otherwise it rejects categories that have no directly configured appenders. I was originally just adding inheritance to the cloned config that gets passed to validation, before I realized that left the original config without any inheritance.
Would it make sense to clone the incoming readonly config, and use that clone thereafter? ie: apply inheritance, validate it, and use it for subsequent getLogger calls.
Another approach I considered was augmenting the categories object, with a getter method for appenders and level that considered inheritance, but I though it would be more efficient to just collapse inherited 'stuff' to sub-categories at startup.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, the steps should be the same as on the master branch for log4js.js:
// log4js.js: line 62
configuration.configure(deepClone(configObject));
Then in configuration.js we add an extra set of listeners (addPreProcessingListeners
?), and call those functions with the cloned configObject. They can modify the config before it gets validated. So categories.js would add three config listeners - one for pre-processing (apply inheritance), one for the validation rules, and one for the setup.
The travis CI failure is unrelated to your changes - I'll see if I can work out what's wrong with that test. |
Working on those requested changes. The suggested preProcessingListeners approach seems to work cleanly. Now reworking unit tests to verify the results. |
for preparing config before validation and interpretation
…nto inherit-parent-category-config
I added requested changes: applying inheritance through preProcessingListener. |
Published to NPM in version 4.2.0 |
Great! BTW, this was a milestone for me: my first-ever open source contribution. |
Congratulations, and thanks again for your help. |
These changes:
See configuration-inheritance-test.js for examples.