[GPT-526] Dynamic requires of oc-templates #415
Conversation
client/src/get-compiled-template.js
Outdated
| if (type === 'jade') { type = 'oc-template-jade'; } | ||
| if (type === 'handlebars') { type = 'oc-template-handlebars'; } | ||
|
|
||
| // dynamically require specific oc-template |
There was a problem hiding this comment.
I would remove this comment. If this is the only thing in the try, I think it's quite clear what's happening here.
client/src/get-compiled-template.js
Outdated
| var ocTemplate; | ||
| try { | ||
| if (type === 'jade') { type = 'oc-template-jade'; } | ||
| if (type === 'handlebars') { type = 'oc-template-handlebars'; } |
There was a problem hiding this comment.
can we move these ifs out of the try so it's clear the fail can happen only on the require line
client/src/get-compiled-template.js
Outdated
| throw format(settings.templateNotSupported, type); | ||
| } | ||
|
|
||
| cb(null, ocTemplate.getCompiledTemplate(templateText, template.key)); |
There was a problem hiding this comment.
I am thinking that these 5 lines of code are actually repeated 4 times. Given we need them for both npm modules (oc and oc-client) - what about moving it to its own npm module, like oc-require-templates so that we can just require it all the times? If that doesn't require too much time obviously?
If not, I would just move it to its own file, like oc-require-templates.js and duplicate it inside the /client and /src folders.
There was a problem hiding this comment.
I would go with #2 for the moment so that we don't spread around too many pieces. Once out of 'flux', this could definitely go in its own npm module.
|
@matteofigus all points should be addressed |
No description provided.