Skip to content
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

feat(compiler): add support for external templates #219

Merged
merged 1 commit into from
Aug 2, 2020

Conversation

tripodsan
Copy link
Contributor

@tripodsan tripodsan commented Jul 22, 2020

first draft to implement #217

@github-actions
Copy link

This PR will trigger a minor release when merged.

@jantimon
Copy link
Contributor

jantimon commented Jul 29, 2020

This looks really cool 👍

Just one small thing the JSPureTemplate has $scriptId which isn't used at all:

module.exports = function main(runtime, $scriptId) {
const $ = {

One bug I found is if a template file is using itself:

<template data-sly-template.headline="${@ text}">
  <sly data-sly-call="${headlineInner @ body=text}"/>
</template>

<template data-sly-template.headlineInner="${@ body}">
  <p>${body}</p>
</template>

Which generates:

  $.template('cdc516118141547c6c395bd7eaa740a3', 'headline', function* _template_cdc516118141547c6c395bd7eaa740a3_headline(args) { 
    const text = args[1]['text'] || '';
    let $t, $n = args[0];
    $.dom.text($n,"\n  ");
    const myModel = yield $.use($use_0, {});
    $.dom.text($n,"\n  ");
    $t = $.dom.create("h1",false,false);
    $.dom.text($n,"\n  ");
    yield $.call(headlineInner, [$n, {"body": text, }]); // <-- 🐛 headlineInner is not defined
    $.dom.text($n,"\n");
  });

  $.template('cdc516118141547c6c395bd7eaa740a3', 'headlineInner', function* _template_cdc516118141547c6c395bd7eaa740a3_headlineInner(args) { 
    const body = args[1]['body'] || '';
    let $t, $n = args[0];
    $.dom.text($n,"\n  ");
    $t = $.dom.create("p",false,false);
    $n = $.dom.push($n,$t);
    const var_1 = yield $.xss(body, "html");
    $.dom.append($n, var_1);
    $n = $.dom.pop($n);
    $.dom.text($n,"\n");
  });

@tripodsan tripodsan force-pushed the external-templates branch from a463b95 to 1ed8253 Compare July 30, 2020 08:55
@tripodsan
Copy link
Contributor Author

One bug I found is if a template file is using itself:

this is indeed a problem... I'll see if we can fix it here or if we should create a follow up bug.

@tripodsan
Copy link
Contributor Author

Just one small thing the JSPureTemplate has $scriptId which isn't used at all:

since it's not needed anymore, we could also use the default code template again....

@tripodsan tripodsan force-pushed the external-templates branch from 1ed8253 to 9ee7a97 Compare July 30, 2020 08:58
Comment on lines +16 to +26
const $ = {
col: runtime.col,
exec: runtime.exec.bind(runtime),
xss: runtime.xss.bind(runtime),
listInfo: runtime.listInfo.bind(runtime),
use: runtime.use.bind(runtime),
slyResource: runtime.resource.bind(runtime),
call: runtime.call.bind(runtime),
template: runtime.template.bind(runtime),
dom: runtime.dom,
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would it be possible to replace this part with const $ = runtime or const $ = runtime.templateHelpers()?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's tackle this in a separate PR

@tripodsan tripodsan force-pushed the external-templates branch from 9ee7a97 to 4a0538e Compare August 2, 2020 09:03
@adobe adobe deleted a comment from lgtm-com bot Aug 2, 2020
@adobe adobe deleted a comment from lgtm-com bot Aug 2, 2020
@tripodsan tripodsan marked this pull request as ready for review August 2, 2020 09:05
@adobe adobe deleted a comment from lgtm-com bot Aug 2, 2020
@codecov
Copy link

codecov bot commented Aug 2, 2020

Codecov Report

Merging #219 into master will increase coverage by 0.30%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #219      +/-   ##
==========================================
+ Coverage   83.75%   84.06%   +0.30%     
==========================================
  Files          87       89       +2     
  Lines        3847     3909      +62     
==========================================
+ Hits         3222     3286      +64     
+ Misses        625      623       -2     
Impacted Files Coverage Δ
src/compiler/Compiler.js 99.39% <100.00%> (+1.29%) ⬆️
src/compiler/DomHandler.js 100.00% <100.00%> (ø)
src/compiler/ExternalTemplateLoader.js 100.00% <100.00%> (ø)
src/compiler/JSCodeGenVisitor.js 98.33% <100.00%> (+0.41%) ⬆️
src/parser/commands/ExternalCode.js 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 998824a...4a0538e. Read the comment docs.

<h1>${data.title}</h1>
<sly data-sly-call="${separator}"/>
</template>
<template data-sly-template.separator>--------------------------------------</template>
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jantimon Here, I added a test for a template that uses a template that is later defined in the same script. and it works now :-)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really cool 👍

@tripodsan tripodsan merged commit 90e20de into master Aug 2, 2020
@tripodsan tripodsan deleted the external-templates branch August 2, 2020 10:09
adobe-bot pushed a commit that referenced this pull request Aug 2, 2020
# [5.1.0](v5.0.0...v5.1.0) (2020-08-02)

### Features

* **compiler:** add support for external templates ([#219](#219)) ([90e20de](90e20de))
@adobe-bot
Copy link

🎉 This PR is included in version 5.1.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants