-
Notifications
You must be signed in to change notification settings - Fork 69
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
APP-3110 Handlebars template implementation #260
APP-3110 Handlebars template implementation #260
Conversation
return getTemplateFromFile(templatePath); | ||
final String directory = FilenameUtils.getFullPathNoEndSeparator(templatePath); | ||
final String file = FilenameUtils.getName(templatePath); | ||
final Configuration configuration = createConfiguration(); // for thread-safety, we need to re-create configuration |
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.
Thanks for putting this useful, because FreeMarker does not recommend to create a new configuration each time we load a template.
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.
The risk here was to update static configuration at method call level, it could be problematic in multi-threaded environment such as Spring Boot applications.
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.
LGTM
@@ -53,11 +53,11 @@ If you want to use [Maven](https://maven.apache.org/) as build system, you have | |||
</dependency> | |||
<dependency> | |||
<groupId>com.symphony.platformsolutions</groupId> | |||
<artifactId>symphony-bdk-core-invoker-jersey2</artifactId> | |||
<artifactId>symphony-bdk-http-jersey2</artifactId> <!-- or symphony-bdk-http-webclient --> |
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.
maybe we should update that according to the discussion we had about runtime scope?
docs/message.md
Outdated
but we may add support for other template engines. | ||
For instance, if you have the following template file: | ||
### How to send a message from a template | ||
> In the code examples below, we will assume that FreeMarker as been selected as template engine implementation |
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.
should we detail how the selection is made (i.e whatever is in the classpath is used)?
One could think that there is an API to call to select the engine
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 documented how to select the implementation. And I also created APP-3113
to fix the current behaviour which was to randomly decide which implementation is selected by default
…mphony-bdk-core transitive dependencies
* APP-3110 Handlebars template engine implementation * APP-3110 doc * APP-3110 Post merge, updated getting-started guide * APP-3110 http and template implementations as runtime dependencies * APP-3110 reverted ./docs/index.md * APP-3110 Updates implementation selection * APP-3110 :symphony-bdk-http-api and :symphony-bdk-template-api as :symphony-bdk-core transitive dependencies (cherry picked from commit edb1844)
* APP-3110 Handlebars template engine implementation * APP-3110 doc * APP-3110 Post merge, updated getting-started guide * APP-3110 http and template implementations as runtime dependencies * APP-3110 reverted ./docs/index.md * APP-3110 Updates implementation selection * APP-3110 :symphony-bdk-http-api and :symphony-bdk-template-api as :symphony-bdk-core transitive dependencies (cherry picked from commit edb1844)
* APP-3082 Architecture Overview (#258) * APP-3082 SVG Rendering in Markdown test * APP-3082 Updated diagram * APP-3082 Removed unused Jersey dependency * APP-3082 WIP * APP-3110 arch doc * APP-3110 removed unused javax.ws.rs.core.GenericType import from ApiException class * APP-3082 fixes * APP-3082 Link to Architecture Presentation from ./docs/index.md (cherry picked from commit 0fa637b) * APP-3110 Handlebars template implementation (#260) * APP-3110 Handlebars template engine implementation * APP-3110 doc * APP-3110 Post merge, updated getting-started guide * APP-3110 http and template implementations as runtime dependencies * APP-3110 reverted ./docs/index.md * APP-3110 Updates implementation selection * APP-3110 :symphony-bdk-http-api and :symphony-bdk-template-api as :symphony-bdk-core transitive dependencies (cherry picked from commit edb1844) * APP-3078: Add java-library plugin to http and template submodules (#261) * Add internal dependencies version to bom module * Update missing dependencies * Adding plugin to http and template submodule (cherry picked from commit f5c7df0) * Bump release version to 1.3.1.BETA * Updated current version in getting-started.md * APP-3110 Handlebars template implementation (#260) * APP-3110 Handlebars template engine implementation * APP-3110 doc * APP-3110 Post merge, updated getting-started guide * APP-3110 http and template implementations as runtime dependencies * APP-3110 reverted ./docs/index.md * APP-3110 Updates implementation selection * APP-3110 :symphony-bdk-http-api and :symphony-bdk-template-api as :symphony-bdk-core transitive dependencies (cherry picked from commit edb1844) * Bumped legacy version Co-authored-by: symphony-hong <65538951+symphony-hong@users.noreply.github.com>
Ticket
APP-3110
Description
Handlerbars implementation for the Template API for adoption purpose, as it is the default implementation provided by legacy sms-sdk-renderer-java library.
I also decided to simplify this API by removing the built-in templates feature that is not very useful at the moment, as we are not really providing any real built-in template. We might reconsider this feature in the future, depending on developers needs for that, but for now I really think that we don't necessary need to publish it in order to avoid any question like "why it exists but there is only 1
simpleMML
template?".Checklist