[GPT-565] Dynamic support to oc-templates in oc dev #426
Conversation
client/src/utils/require-template.js
Outdated
| throw format(templateNotSupported, type); | ||
| module.exports = function(template) { | ||
| var localTemplate = path.join(__dirname, '../../', 'node_modules', template); | ||
| if (fs.existsSync(localTemplate)) { |
There was a problem hiding this comment.
we can't use sync functions in the client. I think the only way to not make all of this async is to bring each require inside own try catch. In the catch, also, you may need to invalidate the require cache. Very similar to what we do here https://github.com/opentable/oc/blob/master/src/cli/domain/get-missing-deps.js#L22
|
|
||
| _.forEach(_.keys(pkg.dependencies), function(d){ | ||
| if (!deps.templates[type] && !legacyTemplates[type]) { |
There was a problem hiding this comment.
I think
var legacyTemplates = ['jade', 'handlebars']
...
if (!deps.templates[type] && !_.includes(legacyTemplates, type)) {would be a bit more readable?
There was a problem hiding this comment.
very nit and very arguable weather an array of 2 elements is more readable then an object with 2 keys.. I've removed the usage of _.include when reformatting this, we were doing way too many useless array loops, so, my rationale is: quicklookups ftw
There was a problem hiding this comment.
That's why I put it as question. I don't have strong opinions on this. Let's leave it as is if you like.
src/utils/require-template.js
Outdated
| throw format(templateNotSupported, type); | ||
| module.exports = function(template) { | ||
| var localTemplate = path.join(__dirname, '../../', 'node_modules', template); | ||
| if (fs.existsSync(localTemplate)) { |
There was a problem hiding this comment.
same as before. Can't use blocking code in the registry
client/src/utils/require-template.js
Outdated
| // } | ||
|
|
||
| // throw format(templateNotSupported, template); |
There was a problem hiding this comment.
forgot to remove these lines?
|
@matteofigus should be ok, let me know if you see anything else that need moar 💅 |
|
|
||
| _.forEach(_.keys(pkg.dependencies), function(d){ | ||
| if (!deps.templates[type] && !legacyTemplates[type]) { |
There was a problem hiding this comment.
Add check that if templatetype is not legacy must be declared as dependency
There was a problem hiding this comment.
Also,
-
on the client, let's ensure that when we call require-template (which can throw) that is wrapped in a try/catch so in case we return a callback with an error: https://github.com/opentable/oc/blob/ff87386f8331b1c257381177f11dd434b300d10c/client/src/template-renderer.js#L16
src/registry/domain/repository.js
Outdated
| info = require(template).getInfo(); | ||
| } catch (err) { | ||
| throw new Error(format(strings.errors.registry.TEMPLATE_NOT_FOUND, template)); | ||
| } |
There was a problem hiding this comment.
Let's re-add a try catch here for a template that, in case is not valid, may throw as getInfo doesn't exist or is not a function. Error wouldn't be "template not found" anymore but more of a "template not valid" kind?
client/src/utils/require-template.js
Outdated
| @@ -1,12 +1,56 @@ | |||
| 'use strict'; | |||
|
|
|||
| var format = require('stringformat'); | |||
| var templateNotSupported = 'Error loading component: template "{0}" not supported'; | |||
| var path = require('path'); | |||
| var _ = require('underscore'); | |||
There was a problem hiding this comment.
we don't have underscore in the client! Made the same mistake when reviewing the previous PR with @mattiaerre and then realised - are you able to go vanilla with client's code?
There was a problem hiding this comment.
Done, actually I like it even more without it.
No description provided.