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

Layout support #62

Closed
whitneyit opened this issue Feb 3, 2015 · 28 comments
Closed

Layout support #62

whitneyit opened this issue Feb 3, 2015 · 28 comments
Labels
enhancement New feature or request

Comments

@whitneyit
Copy link
Contributor

I've started to implement layout support for ejs and before I go too far down the rabbit hole I thought it would be worthwhile to get some feedback.

In my fork of the repo I have started to add some test file for the possible layout syntax. The idea behind the syntax is based off of Laravel's templating engine.

I have put together a few example use cases that will need to be tested against.

Layout files

Use cases

What do you think?

@mde
Copy link
Owner

mde commented Feb 3, 2015

This uses template inheritance?

@whitneyit
Copy link
Contributor Author

Yes. The goal is to have a system in which you can nest templates and have that partial be extended.

@mde
Copy link
Owner

mde commented Feb 3, 2015

This looks really interesting, but it seems like it's pretty far outside the purview of the EJS library. It does provide a simple include feature, but that's mostly because it's a very lowest-common-denominator feature that's pretty intimately tied to template compilation and execution.

I would be happy to help with making an extension like this work well with EJS -- even possibly exposing something you can plug into that makes things easier. But I don't think this belongs in the core lib.

@whitneyit
Copy link
Contributor Author

Yea I looked into include and have used it before but it doesn't provide enough flexibility for what I was using it for.

A plugable interface works fine so far as I am concerned. Laravel has a similar setup to allow for extensions.

Is this something that is on the roadmap?

@mde
Copy link
Owner

mde commented Feb 3, 2015

It is now. Or it will be, if you can help us stub out what it is and how it should work. :)

@whitneyit
Copy link
Contributor Author

Yea man no problem. I'll put together some architecture to allow for plugging.

@TimothyGu
Copy link
Collaborator

To be honest, I think this is getting too much like Jade. From an implementation PoV it is hard to implement the "commands" like extends, etc. What you can work towards is something like this which is admittedly more spartan but easier to implement:

<% extends('templ', function (section) { %>
  <% section('title', function () { %>
    <h1>Hola</h1>
  <% }) %>
  <% section('body', function () { %>
    <p>¿Cómo estás?</p>
    <p>Bien, ¿y ?</p>
  <% }) %>
<% }) %>
<% layout('templ', function (section) { %>
  <%- yield('title') %>
  <%- yield('body') %>
<% }) %>

@whitneyit
Copy link
Contributor Author

@TimothyGu that's a pretty good take on it. I'll give it a shot tonight

@TimothyGu
Copy link
Collaborator

@whitneyit Good luck :)

@TimothyGu TimothyGu added the enhancement New feature or request label Feb 3, 2015
@whitneyit
Copy link
Contributor Author

As part of implementing the layout functions, I decided to clean up the compile source of the generated compile function. When running tests with {debug: true} set and the function was logged to the console it initially looked like this.

var __line = 1, __lines = "<style><% var value = 'bar' %><% include style.css %></style>", __filename = "test/fixtures/include_preprocessor.css.ejs"; try {var __output = ""; with (locals || {}) { ;__output += "<style>";; var value = 'bar' ;(function(){;__output += "body {\n  foo: '";;__line = 2;;__output += escape(value);__output += "';\n}";;__line = 3;})();;__output += "</style>";};return __output.trim();} catch (e) { rethrow(e, __lines, __filename, __line); }

It now looks like this

var __line = 1
  , __lines = "<style><% var value = 'bar' %><% include style.css %></style>"
  , __filename = "test/fixtures/include_preprocessor.css.ejs";
try {
  var __output = "";
  with (locals || {}) {
    __output += "<style>";
    var value = 'bar';
    (function(){
    __output += "body {\n  foo: '";
    __line = 2;
    __output += escape(value)
    __output += "';\n}";
    __line = 3;
    })();
    __output += "</style>";
  }
  return __output.trim();
} catch (e) {
  rethrow(e, __lines, __filename, __line);
}

I just formatted the lines and removed the extra semicolons. Is this something that you guys would want as a separate PR? Apart from asthetics the only functional difference was that I added a .trim() to the line eval.

@TimothyGu
Copy link
Collaborator

@whitneyit

First of all, always rebase your commits on the master branch. development branch is reserved for backwards-incompatible code.

Technically putting semicolons in front (only) is more robust, in order to keep semicolon-less code working now and in the future.

The .trim() isn't really necessary, and I would prefer less speed-affecting functions especially in the already-slow compilation process.

I don't really care about whitespace, but EJS v1 did beautify the sources. Why break something that worked?

Also always remember to run npm test to make sure your changes don't break anything. I'm not saying it did break something but it's possible.

@whitneyit
Copy link
Contributor Author

I ran all of the tests and everything passed fine. I don't understand what
you mean by

 Why break something that worked?

I'll implement your suggestions and continue to use the extra whitespace
for debugging whilst tinkering with the layout code.
On 4 Feb 2015 4:13 am, "Timothy Gu" notifications@github.com wrote:

@whitneyit https://github.com/whitneyit

First of all, always rebase your commits on the master branch. development
branch is reserved for backwards-incompatible code.

Technically putting semicolons in front (only) is more robust, in order to
keep semicolon-less code working now and in the future.

The .trim() isn't really necessary, and I would prefer less
speed-affecting functions especially in the already-slow compilation
process.

I don't really care about whitespace, but EJS v1 did beautify the sources.
Why break something that worked?

Also always remember to run npm test to make sure your changes don't
break anything. I'm not saying it did break something but it's possible.


Reply to this email directly or view it on GitHub
#62 (comment).

@TimothyGu
Copy link
Collaborator

When I said

Why break something that worked?

I meant EJS v2 removed something that worked in EJS v1.

@whitneyit
Copy link
Contributor Author

Ok mate I wasn't sure if that's what you meant or if I you meant that I
would be implementing breaking changes

Cheers
On 4 Feb 2015 7:29 am, "Timothy Gu" notifications@github.com wrote:

When I said

Why break something that worked?

I meant EJS v2 removed something that worked in EJS v1.


Reply to this email directly or view it on GitHub
#62 (comment).

@whitneyit
Copy link
Contributor Author

@TimothyGu I reformated the output of the compile function with your suggestions. I removed the .trim() and inserted semi colons only at the front of each statement. The code is rebased off of master and I can setup a PR if wanted.

The output now looks like:

var __line = 1
  , __lines = "<style><% var value = 'bar' %><% include style.css %></style>"
  , __filename = "test/fixtures/include_preprocessor.css.ejs";
try {
  var __output = [];
  with (locals || {}) {
    ; __output.push("<style>")
    ;  var value = 'bar'
    ; (function(){
    ; __output.push("body {\n  foo: '")
    ; __line = 2
    ; __output.push(escape(value))
    ; __output.push("';\n}")
    ; __line = 3
    ; })()
    ; __output.push("</style>")
  }
  return __output.join("").trim();
} catch (e) {
  rethrow(e, __lines, __filename, __line);
}

@TimothyGu
Copy link
Collaborator

@whitneyit said:

I noticed that you updated the test that was generating the output above

I'm not 100% sure what you are referring to here.

Please do make a PR. This looks much nicer than the original output.

@whitneyit
Copy link
Contributor Author

I edited my comment because I noticed that you addressed issue #60 and it looked like the output was different but I was mistaken.

@TimothyGu
Copy link
Collaborator

@whitneyit #63 does change the code in this area, but it is not merged yet and I'll fix any merge conflicts.

@TimothyGu
Copy link
Collaborator

I just found out about https://github.com/JacksonTian/ejs-mate.

It surely looks fairly promising, although some work might be necessary to adapt it to EJS v2.

@whitneyit
Copy link
Contributor Author

Oh wow that's really cool. I had basic support for layouts working already.
I'll investigate that repo and take something from it.

Cheers for the link :)

On 5 February 2015 at 12:03, Timothy Gu notifications@github.com wrote:

I just found out about https://github.com/JacksonTian/ejs-mate.

It surely looks fairly promising, although some work might be necessary to
adapt it to EJS v2.


Reply to this email directly or view it on GitHub
#62 (comment).

Regards,
James

@btknorr
Copy link

btknorr commented Apr 13, 2015

Any ideas when layout support will be ready? We are needing this asap. Thanks!

@whitneyit
Copy link
Contributor Author

I don't have the time to work on this just yet. Its still on my list of
things to do, and I need it for an upcoming leg of the project I am working
on. So it will be done, but unfortunately I cannot give you a fixed date.

By all means feel free to contribute :)

Regards,
James
On 14 Apr 2015 06:47, "Brian Knorr" notifications@github.com wrote:

Any ideas when layout support will be ready? We are needing this asap.
Thanks!


Reply to this email directly or view it on GitHub
#62 (comment).

@LinusU
Copy link

LinusU commented Apr 20, 2015

I would love this! Thanks for taking time for this and if you want someone to discuss implementation details with just post here :)

@ardoramor
Copy link

Any ETA updates on this sweet enhancement?

@JpEncausse
Copy link

Hi,
I'm using ejs-locals (https://github.com/randometc/ejs-locals) that provides layouts for EJS.
It works with EJS from VisionMedia (https://github.com/tj/ejs)
It breaks with your EJS

Any advices to make it works ?

@RyanZim
Copy link
Collaborator

RyanZim commented May 5, 2016

@whitneyit Are you still working on this?

@RyanZim
Copy link
Collaborator

RyanZim commented May 30, 2016

@whitneyit ping?

If I don't hear from you within ~7 days, I will close this.

@whitneyit
Copy link
Contributor Author

I made some progress on this but I no longer have access to the client's machine where the work was being done and again I do not have the spare time to work on this.

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

No branches or pull requests

8 participants