STRF-4237: Split off rendering functionality #130
Conversation
|
@bigcommerce/storefront-team |
index.js
Outdated
| case 'handlebars-v4': | ||
| this.renderer = new HandlebarsRenderer(this.siteSettings, this.themeSettings, 'v4'); | ||
| break; | ||
| case 'handlebars': |
There was a problem hiding this comment.
Can we just use default 🍹
There was a problem hiding this comment.
I'd prefer to keep this explicit, even just as documentation
index.js
Outdated
| */ | ||
| loadTemplates(path, callback) { | ||
| let processor = this.getTemplateProcessor(); | ||
| let preProcessor = this.renderer.getPreProcessor(); |
There was a problem hiding this comment.
Can we move this call to the constructor as a variable so we don't make this call for each path in the list ?
junedkazi
left a comment
junedkazi
left a comment
There was a problem hiding this comment.
👏 Really excited about this PR as it will open up possibilities in terms of all different templating engines we can support.
* Split off actual rendering functionality into new module called paper-handlebars. This encapsulates rendering with Handlebars v3. This split allows us to introduce Handlebars v4 and potentially other template engines in the future, without changing the Paper interface.
|
@bigcommerce/storefront-team ♻️ |
| "license": "BSD-4-Clause", | ||
| "scripts": { | ||
| "test": "lab -l -t 90" | ||
| "linter": "eslint .", |
There was a problem hiding this comment.
yay! i love the minimalism of using npm scripts over grunt/gulp tasks. :)
| helpers.forEach(helper => helper(this)); | ||
| } | ||
| // Build renderer based on type | ||
| switch(rendererType) { |
There was a problem hiding this comment.
🍹 part of me wants to reduce this to something like:
this.renderer = new HandlebarsRenderer(this.siteSettings, this.themeSettings, rendererType.split(/-/).pop();
but being explicit is probably a better idea
| - 4 | ||
| - 6 | ||
|
|
||
| - 8 |
There was a problem hiding this comment.
Should we drop node 4 from the list ? Since all our prod & dev envs are node 6 & above ?
There was a problem hiding this comment.
We still need to support CLI, which supports Node 4 still.
3 participants
rendererTypeparameter to Paper constructor. This defaults tohandlebars(the current v3 implementation). If you passhandlebars-v4, it will use 4.0.11.~instead of^and updated dependencies to match what we're currently using.Breaking changes
contentServiceContextfor setting page content. From now on, useaddContent()andgetContent().getTemplateProcessor(). This is an internal concern ofpaper-handlebarsand is used byloadTemplates.loadTemplatesSync(). This was only used by helper tests and is no longer needed.handlebarsinstance variable. Hopefully nobody is accessing that directly. Any helpers that were accessing it have been updated inpaper-handlebarsto use the global context they are given rather than accessing Paper directly at all.translatorattribute has been moved topaper-handlebarsand is no longer accessible directly on Paper.decoratorsattribute has been moved topaper-handlebarsand is no longer accessible directly on Paper.settingsattribute has been renamed tositeSettings. This should only be accessed bypaper-handlebars.cdnify()function has been moved into a helper library inpaper-handlebars.injectattribute has been removed. This is storage used by two of the helpers, and the implementation has moved topaper-handlebars.