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

block support for ejs #251

Closed
wants to merge 10 commits into from
Closed

block support for ejs #251

wants to merge 10 commits into from

Conversation

huxia
Copy link

@huxia huxia commented Apr 17, 2017

@RyanZim
Copy link
Collaborator

RyanZim commented Apr 17, 2017

@mde @TimothyGu Thoughts on this?

I'm not sure if I like this approach, but I'm not sure what's the best approach (or if we should ever add this).

Either way, this is a breaking change.

2. less output for non-client-mode
@huxia
Copy link
Author

huxia commented Apr 17, 2017

@RyanZim
Thanks for your suggestion.

I agree that there should be no breaking changes. I'll add a option to make this feature configurable.

You have a very good listing of questions and priorities:

  1. whether we should add layout/template support for ejs
  2. is it the right approach
  3. is it the right implementation

my idea is, template/layout is very important for server rendering. (I was using https://github.com/koajs/ejs before and surprisingly I found the "layout feature" was not by provided by ejs, plus, it only support a single "body" block; https://github.com/Soarez/express-ejs-layouts is better, but its weakness is, it supports only one time of layering, and the block grammar is confuse to me)

I'm also not sure whether it's the right approach.

I think the goal here is to keep ejs simple while we make it powerful, so I tried to:

  • have a javascript-compliant grammar
  • introduce new preprocessor directives as little as possible

As for the implementation, I'm sure there are many places to optimize & correct.

@RyanZim
Copy link
Collaborator

RyanZim commented Apr 17, 2017

The basic question/problem with adding layouts is that it runs counter to the way EJS works. EJS does most of its job at runtime. When we introduce layouts, most implementations require compiling the layouts at compile-time. Your implementation gets around this by requiring the user to include the layout from the template.

However, I'm not sure if I like that approach. I forsee users getting confused by it.

The other concern I failed to mention is keeping EJS small and fast. It's one of the faster JS template engines that is turing-complete (i.e. not logic-less like mustashe). It's also possible to compile and render templates on the client-side due to its small size (only 19KB minified, smaller when gzipped). We want to keep it that way.

What I'm wondering is: can't this be implemented as a wrapper around EJS?

@huxia
Copy link
Author

huxia commented Apr 17, 2017

@RyanZim
option.blocks added.

the approach won't break client support for EJS, I've already added a test case: https://github.com/mde/ejs/pull/251/files#diff-e609a7dc3a37943002296475c8bd7306R1041

for speed, there should be no performance impact if blocks is not used; on the other side, a single operation <% var a = block(function(){ ... }); %> 's time usage should be no more than a <% include(...) %>

can't this be implemented as a wrapper around EJS?

I think EJS would need to be modified to support such wrappers

@mde
Copy link
Owner

mde commented Apr 18, 2017

I will take a closer look at this -- but first off, when @RyanZim talks about "client-side," he's not referring to the client: true option, but usage of EJS in the browser. This change would introduce a non-trivial extra download cost for everyone using EJS in the browser, even if they don't intend to use blocks.

As far as implementing it as a wrapper -- why would EJS need to be modified? You mentioned express-ejs-layouts, but it appears to use just stock EJS. (Maybe modifying EJS would be required for it to recurse?) Also, I'm not opposed to modifying EJS to make it easier to wrap/extend. I would be really interested in how we could make it easier for people to do that, since there seems to be lots of people who have very specific ideas of extra things they wish EJS would do.

@c3y
Copy link

c3y commented Oct 2, 2017

@RyanZim hello, I am bit confused. it may be a dump question or wrong place since i am a startup, but do you mean that layout/extend features doesn't add value to a template engine? may i ask what is your suggestion for a workaround? Rewriting same sections of code for each page?

thanks a lot..

@ViktorQvarfordt
Copy link

I think we all agree this feature is a must and should be highly prioritized. What are we stuck on? How can I help? I very much want this feature implemented.

If @RyanZim decision on not including this in the main ejs file is final, I suggest that this feature should still be "ejs official" and bundled with this package. That is, writing and official wrapper 'ejs-layout'. Right now we have multiple diverging implementations popping up.

@ViktorQvarfordt
Copy link

I think we should consider the implementation https://github.com/Soarez/express-ejs-layouts as well.

dashi0 added 2 commits January 3, 2018 13:42
* offical/master: (25 commits)
  Add Node 9, restore older stable versions
  Escape filename for error when unable to find include file
  add node 9 to travis environments
  update to latest version
  update eslint to latest and fix issues
  Move leading comma (#313)
  added a test for including nonexistent files
  showing the name of the include file that can't be found
  Version 2.5.7
  Updated README for release
  test for include in expression or escaped
  Include linting tests in test script command
  Rename benchmark file in the interest of getting a release out
  fix sorting
  revert 8aaa120 use only one __output array for template and includes
  benchmark
  Tests for multiple view directories
  Implement multiple view directories for include
  Linting
  rename snake to camelCase
  ...

# Conflicts:
#	lib/ejs.js
@huxia
Copy link
Author

huxia commented Oct 10, 2018

I have a better idea for introduce block/template support for ejs, without inventing new "block"/"blocks" built-in function. So I'll close this PR.

See #252 for discussion.

As for question raised by @mde, should ejs be changed or should an "ejs-extension" method be invented. My thought is: block/template is a key feature for an template-engine; learning & finding extensions will create burdens for developers; compare to other template-engine, ejs's advantage is developers needs less study & knowledge other that pure javascript gramma.

So I think ejs should have its own block/template feature.

@RyanZim @mde

@huxia huxia closed this Oct 10, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants