Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

When a namespace is specified, register partials in that namespace #33

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
4 participants
Contributor

pdokas commented Feb 22, 2013

A namespace is specified for many reasons, most involving the idea of being able to dynamically look up templates at runtime. There are cases where it becomes important to also have access to partials using the same mechanism that’s used for templates.

This PR modifies the registerPartial() call to additionally add a reference to the partial in the provided namespace. If no namespace is provided then the registerPartial() call is unmodified.

Owner

tkellen commented Feb 22, 2013

Thanks for taking the time to put this PR together Phil!

I've never seen this pattern before, and I think it might muck up the current use case for namespaces and partials.

/cc @tbranyen / @mehcode / @thanpolas / @phawk can I get some more eyes on this?

Contributor

thanpolas commented Feb 22, 2013

@pdokas reasoning makes sense.

However for production optimization reasons i'd like to see that feature only enabled explicitly by setting a debug switch to true in the task's configuration.

Contributor

phawk commented Feb 22, 2013

This isn't going to break any of the current implementations so long as a partials file name doesn't conflict with a templates filename, could become quite difficult to debug if it does.

Would be nice to prefix the partial filename with something to ensure this doesn't happen. 'partial_'+... or something similar/

Other than that I'm happy enough with it.

Contributor

pdokas commented Feb 22, 2013

I spoke with my team member who originally proposed this solution and he presented a good example use for this.

Let’s say you have a single page app where the History API is used to dynamically provide links to states. These URLs can be directly requested, resulting in server-generated page loads.

So on this site you have a page which has a link that opens a modal dialog with the page's metadata in it. This updates the URL from http://foo.com/posts/123/ to http://foo.com/posts/123/meta/. Essentially what happens here is a metadata partial is rendered into the modal dialog.

Now when this URL is bookmarked or shared, what happens in our setup is the server renders a page. And in this setup, instead of rendering the normal /posts/123 page and then somehow directing the client-side end of things to automatically trigger the JS modal dialog state, instead it renders the metadata partial as a full page.

To achieve this deep-linking trick a low-cost engineering solution is to register partials in the standard registerPartial() way, but also to hang them off your custom namespace where your usage patterns are up to the developers. It is this pattern that this PR addresses.

Contributor

thanpolas commented Feb 22, 2013

@pdokas would you discuss the option to have the modal rendered as a normal view instead of a partial?

It sounds like the normal view role is more fitting for this case.

Contributor

pdokas commented Feb 22, 2013

@thanpolas I’m sorry, could you explain what you mean by “normal view”?

Also, I think your idea of a debug flag could also work. Perhaps it could be something along the lines of an option called partialsUseNamespace which is a boolean, or even partialsNamespace which is a string and could be used to avoid the collision possibility @phawk mentioned. Either of those would be fine in my eyes.

Contributor

thanpolas commented Feb 22, 2013

@pdokas the contents of the modal when route /posts/123/meta/ is viewed, should be a handlebars template and not a partial is what i'm suggesting.

Contributor

pdokas commented Feb 22, 2013

@thanpolas I agree, and in essence that’s what this PR is all about 😀

The reason why a partial is the better choice is because its predominant use case is as a transient bit of DOM that’s dynamically loaded and later discarded in the client.

However, when a route is entered that triggers server-side page generation where this partial provides the entirety of the DOM it makes a lot of sense from a DRY perspective to treat the partial as a proper template and render it directly instead of creating an all-new template that consists of:

{{> metadata}}
Contributor

thanpolas commented Feb 22, 2013

I think I failed to explain... my idea was to have no partial in the mix whatsoever.

Nevertheless, if you judge that a partial is the way to go and you need to treat it as a partial on the front and as a template in the back then that's an operational requirement and not a debug one.

I still think this should be optional, so the explicit declaration for the switch needs to change from debug to something more fitting... partials_with_namespaces ?

Contributor

pdokas commented Feb 23, 2013

@thanpolas I agree, do you have a preference between the boolean and string options I suggested four comments back?

The idea being that instead of checking for the presence of the current namespace option before adding partials to the namespace, that we’d either…

  1. if partialsUseNamespace is set to true, add partials to namespace
  2. if partialsNamespace is provided, add partials to it
Contributor

thanpolas commented Feb 23, 2013

I vote for the boolean value.

Namespace collisions between templates and partials should be the responsibility of the developer, to the effect that he / she chooses a proper naming convention.

@tkellen, @tbranyen ball is in your hands

Contributor

pdokas commented Feb 23, 2013

I’m personally ok with both options. I agree that collisions are a developer responsibility in the same vein as inadvertent variable shadowing.

Owner

tkellen commented Feb 24, 2013

I vote for the boolean value also. Let's do that and get this puppy deployed.

Contributor

pdokas commented Feb 24, 2013

Excellent, I'll get a commit in later today.

Owner

tkellen commented Feb 26, 2013

Hey Phil, would you mind rebasing these into a single commit? I'll merge and publish right away after.

Contributor

pdokas commented Feb 27, 2013

All set!

@tkellen tkellen closed this in f7b1733 Feb 27, 2013

@pdokas pdokas deleted the pdokas:partials-namespace branch Mar 6, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment