Add Jambo build validation hook #229
Conversation
- Added another operation inside PageWriter to ensure template data is correct using TemplateDataValidator, which pass in the templateArgument to the theme's validation hook function if it exists J=SLAP-1369 TEST=auto Passed Jest test using default config format with/without missing field when pass to validation hook
yen-tt
force-pushed
the
dev/validation-hook
branch
from
b8d8dc5 to
2fc6aa0
Compare
| try { | ||
| info(`Validating configuration for page "${pageName}".`); | ||
| const validatorFunction = require(this._templateDataValidationHook); | ||
| validatorFunction(pageData); |
There was a problem hiding this comment.
I'm wondering if we want this class and the hook to throw an exception when the data is invalid. The alternative is that both return a boolean. I think the boolean has its advantages:
- Consumers of the validators can more easily decide how they want to handle bad templates. It may be that they thrown an exception if the validation result is
false. Or, they try and handle more gracefully. As an example, in the future, maybe users can configure Jambo to gracefully ignore bad pages and continue generation (although I think the more catastrophic failure is the way to go for now). - We can differentiate between an actual issue running the Theme supplied validator and an invalid template. Right now, if there was an exception thrown because the validator couldn't run properly, we would treat that the same as the validator finding the template to be no good. We should probably treat the two situations differently and give different errors.
There was a problem hiding this comment.
That makes sense, it would give user more flexibility on exception handling. This would fall on the Theme’s hook to log in detail what is invalid, and the pageWriter will throw a new error of general bad template if hook return false. Updated in new commit
src/commands/build/pagewriter.js
Outdated
| } | ||
|
|
||
| /** | ||
| * Writes a file to the output directory per page in the given PageSet. | ||
| * | ||
| * @param {PageSet} pageSet the collection of pages to generate | ||
| * @throws {UserError} on missing config(s) |
There was a problem hiding this comment.
Small nit: It's not so much that the error is thrown for missing config(s) but for template data that fails a Theme's validation. In theory, it could fail for any number of reasons: missing keys, improperly structured data, etc.
There was a problem hiding this comment.
Added that to address existing code (line 59) where it checks for config file, but forgot to include the new errors throw from the validation. Going to update that
* Do not run the babel helper in Development Mode. (#224) (#225) * Use npmlog for logging with colors (#228) * Add Jambo build validation hook (#229) * Added acceptance testing framework + tests * add --globs param to `jambo extract-translations` (#233) * Pass partials to templatedatavalidator (#237) * `jambo upgrade` no longer deletes the themes folder if there is an error when git checkout-ing the new branch (#235) * Update canonicalizeLocale to handle 中文(chinese) (#239)
using TemplateDataValidator, which pass in the formatted config data to the theme's validation
hook function
J=SLAP-1369
TEST=auto
Created Jest tests using default config format with/without missing field when
pass to validation hook and check if errors are handle properly