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/mixin (again) #274

Open
User4martin opened this issue Jul 13, 2017 · 7 comments
Open

block/mixin (again) #274

User4martin opened this issue Jul 13, 2017 · 7 comments

Comments

@User4martin
Copy link

I am aware of #251 and #252

Please have a look at:
https://github.com/User4martin/ejs/tree/template_subclass

This is not production ready / just proof of concept

It entirely optional:

  • It does not increase the size of the main ejs.js file / comment block support for ejs #251#issuecomment-294512048 about client side use
  • If not activated, then no syntax changes, no existing ejs gets broken. If not activated, then there is no performance change either.

The main change to ejs is to make the Template class available, and allow to substitute it, This will allow for any sort of plugin.
(And maybe Template.modes.* should be exported separately (or on the prototype), so subclasses do not need to copy them)

I would (for better maintenance) like to see the file (when ready) included in the distribution. And maybe plugins for other features too.
But if not then at least make plugins possible by the changes in ejs.js itself.


About the ejs-mixin

This is fully up for discussion.

  • The syntax for defining blocks can be changed if anyone has a better idea.
  • The use of "include()" can and should be replaced.

About the "include": There should probably be a <% mixin(name, locals) %> call instead.
That can be done, but it would need some more changes to ejs.

The Template class would need a function, that returns a list of name/function pairs, that can be used.

Template.prototype.commands = function(data, opts) {
    return { inline: function()... {}, };
}

and there could be an additional
prepended += ' with (argument_commands || {}) {' + '\n';

That way sub classes can add commands.

@mde
Copy link
Owner

mde commented Jul 16, 2017

A good step toward making EJS extensible, which might give us a better answer for when people have specific functionality they think needs to be added. Would be happy to work with you for some forward progress on this.

@User4martin
Copy link
Author

Great, looking forward.

I will look at how to implement my "mixin", and also try to port ejs-locals to an add-on of ejs.
I will revert when I have more of a demo.

I may have some other ideas of changes/improvements along the way. But I will open separate issues, if any of them materializes.

@mde
Copy link
Owner

mde commented Jul 16, 2017

One tiny thought: "mixin" has some fairly specific meaning already:

https://en.wikipedia.org/wiki/Mixin

Basically a bag of properties included (in JS, often copied onto) in an object, without being inherited from a parent class.

I would want to see how include is changed, but mixin doesn't seem like the right terminology.

(Worth nothing also that 'template inheritance' is a thing that exists, too, but seems very different from the 'inheritance' here where we're talking about subclassing the base Template.)

@User4martin
Copy link
Author

I pushed an update version for discussion.

I renamed it to "snippet". I know "block" would be best. But that is used by ejs-locals (which I want to port to a plugin too). So I don't want to call it "block". The name "template" is already used to refer to the entire template, so it would be confusing too. And otherwise I am out of ideas for a good name.


The overall changes in lib/ejs.js are minimal (IMHO).
Also the effect on compilation time is very low (on my system it is not measurable, since it is lower that the variance between to benchmarks of the same code.)


One question upfront: Would you be willing to include the code (as separate js file) in the main distribution?
the following code in lib/ejs.js depending on that (if that concept is ok at all)

exports.enableSnippets = function () {
  require('./ejs-snippet');
};

Otherwise people would just need to require the plugin separately, and the plugin adds itself to the ejs module.


The actual changes needed in lib/ejs.js

  • exports.Template = Template; to make the class accesible
  • add an init method for subclasses
  • add the methods generateArgumentNames and generateArguments.
    Those allow plugins to change the argument list of the generated function.

With this the plugin can now pass in a parameter name "snippet".
So in the template `<%- snippet('foo') %> can call the snippet function.

I also modified the var returnedFn = function so when an include is called, it also receives the generateArguments from the caller (it can then decide to generate new one, or keep them).

That way the top level template can generate ONE function for "snippet", and attach data to that function.
And then all included (and nested included) templates can keep the same "snippet" function and data.

This means that a template defined in one file (e.g. "snips.ejs") can be used

  • in files included by "snips.ejs"
  • in files that included "snips.ejs"


As for the plugin itself:
Current behaviour (which can be changed, but is actually convenient) should be (I still need to write the tests....):

  • Snippets are hoisted within the template in which they are defined.
    • Snippets see the local data, as it was on top of the file (but like include can be given changes to the locals at each call)
  • Snippets defined in an include, are available only after the include was executed
    (after all the filename may be computed at run time)
  • Snippet definition can not be nested
    • However a snippet can include other files, and in those other files new snippets can be defined


Any comments?

@User4martin
Copy link
Author

I added tests, and it works all as expected.

I also made a 2nd version, based on the changes I made in other pull requests (and branches).
https://github.com/User4martin/ejs/tree/template_subclass_regex

This version introduces a new mode <%* for control tags.

  • The mode is added from within the plugin.
  • Other plugins can share this mode.
  • Usage of the mode with unknown content is detected and gives an error
    <%* what %> will give an error at compile time.

@User4martin
Copy link
Author

User4martin commented Jul 29, 2017

OFF TOPIC

I know I added a lot of pull requests (and have some more coming / see branches in my fork).

And probably the expected way would have been, to first check with you, if those changes are within the plans that you have for ejs.

However I wanted to first see, if my ideas work as expected (and benchmark them).

So I am aware that some of them may not be welcome. In which case let me know, so I can rebase the others. (If any remain, but I hope so).

Btw the https://github.com/User4martin/ejs/tree/optimize-combine-append-calls https://github.com/User4martin/ejs/tree/refactor-generate-source-full-tokens together with gives some good speed gain on execution time of the generated function.
It depends a lot of the template. For some it will do nothing. One of the benchmarks gained ~ 40% on my machine (Though that template benefits more than usual, as it has a lot of literals <%% (

@User4martin
Copy link
Author

@mde @RyanZim

I added docs for plugin api and snippets
https://github.com/User4martin/ejs/blob/plugin-snippets/docs/plugins.md
https://github.com/User4martin/ejs/blob/plugin-snippets/docs/plugin-snippet.md

Any comments would be welcome.

As far as I can see the snippets are ready as they are. (docs, test, code)

I am working on another plugin (replacing ejs-locals). This would bring a few minor modifications to ejs. But more later.

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

No branches or pull requests

2 participants