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

Template registration, first implementation (now in dev-3.x) #1026

Closed
wants to merge 0 commits into from
Closed

Template registration, first implementation (now in dev-3.x) #1026

wants to merge 0 commits into from

Conversation

clarle
Copy link
Collaborator

@clarle clarle commented Jul 24, 2013

As discussed in #1021 and the linked gist.

Here's a first drop for template registration, including full tests, user guide documentation, and API docs.

@ericf and I were discussing a possible addition to the Y.Template.register API, where it would also accept a hash of template names to template functions. For example:

// Registers 'foo', 'bar', and 'baz' templates all at once
Y.Template.register({
     'foo': fooTemplate,
     'bar': barTemplate,
     'baz': bazTemplate
});

I started implementing something like this, but I wasn't sure what Y.Template.register(templateHash) would return. The current Y.Template.register(templateName, template) returns the revived template to maintain compatibility with the revive() API.

Pinging @caridy also for review.

/**
Simple cache that maps a template name to the revived template functions.

@property _cache
Copy link
Member

Choose a reason for hiding this comment

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

I'm wondering if _registry is a better name for this property…

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I agree. It makes more sense than _cache, so I'll change it all around.

@caridy
Copy link
Member

caridy commented Jul 24, 2013

About Y.Template.register(templateHash), I don't like it. You will probably use a tool to compile your templates, and a simple each if you really need to register a bunch of them at once, aside from the fact that it will make the api way more complex. Let's stick to the simple use-cases.

@caridy
Copy link
Member

caridy commented Jul 24, 2013

+1

@ericf
Copy link
Member

ericf commented Jul 24, 2013

@clarle I sent you a PR: https://github.com/clarle/yui3/pull/1 with some changes:

  • Remove trailing whitespace
  • Make sure Y.Template.render() always returns a string
  • Added @since tags to API docs
  • Tweak API doc text
  • Tweak user guide text
  • Added HISTROY.md entry

@ericf
Copy link
Member

ericf commented Jul 24, 2013

@clarle here's some improvements on the example based on your comments: https://github.com/clarle/yui3/pull/2

@clarle
Copy link
Collaborator Author

clarle commented Jul 24, 2013

@ericf @caridy Everything looks good to me, is it ready to merge?

@ericf
Copy link
Member

ericf commented Jul 24, 2013

@clarle I think it's ready, but we should wait until the end of the week to give @rgrove a chance to look this over (and anyone else interested) since he was involved in the original discussion.

@tivac
Copy link
Contributor

tivac commented Jul 25, 2013

:shipit:

</dl>

<p>
Using template registration usually involves taking a precompiled template, and wrapping it as a YUI module. You can use packages such as <a href="https://github.com/yahoo/locator-handlebars">locator-handlebars</a> to do so automatically, by having it find the location of your existing templates. The wrapped pre-compiled template should look something like this:
Copy link
Contributor

Choose a reason for hiding this comment

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

There is also the module I wrote, mustache-wax, but it is admittedly not as integrated into the Y.Template sphere as locator-handlebars is.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I like the patterns idea that you have there, though we'll probably do more of that on the server-side compilation step. It might be useful to group templates by their templating engine, if we have a project that has precompiled templates with multiple rendering engines.

Copy link
Member

Choose a reason for hiding this comment

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

@evocateur once this lands, would this be something that you'd use in mustache-wax?

Copy link
Contributor

Choose a reason for hiding this comment

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

@ericf Totally! I've always felt kind of weird by the namespacing options I added to simulate this kind of registry in the past.

@rgrove
Copy link
Contributor

rgrove commented Jul 25, 2013

Thanks for waiting guys. Sorry I couldn't get to this sooner; I'm swamped.

I really like this! I suggested a few minor API doc tweaks above, but otherwise this looks great.

Incidentally, I would love to see multi-template registration like @clarle mentioned in the PR description. I would use that all the time.

templates available from invoking a template to render it. This central
registry and abstraction of templates to names separates concerns, creates a
level of indirection, and enables templates to be easily overridden.
([#1021][])
Copy link
Member

Choose a reason for hiding this comment

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

I realized I forgot to add the URL to this issue…

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll do that when I finish up the API docs changes too.

@clarle
Copy link
Collaborator Author

clarle commented Jul 26, 2013

I think I've addressed any outstanding issues, and I've fixed one small bug in the user guide where engine.revive was passed in the wrong parameter.

Anything else before I merge?

@rgrove
Copy link
Contributor

rgrove commented Jul 26, 2013

:shipit: 🚢

@clarle
Copy link
Collaborator Author

clarle commented Jul 26, 2013

Manually merged into dev-3.x, let me know if any other problems come up!

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