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

Fold template-extension package into core packages #3706

Closed
aldeed opened this issue Feb 11, 2015 · 18 comments
Closed

Fold template-extension package into core packages #3706

aldeed opened this issue Feb 11, 2015 · 18 comments

Comments

@aldeed
Copy link
Contributor

aldeed commented Feb 11, 2015

_2 Upvotes_ I'd like to make a case for folding the template-extension package API into the core API from blaze/templating packages.

  • It's messing with private properties.
  • Some of the functions could be implemented in a more ideal way if done alongside the core functions
  • Most of the functions are extremely useful, to the point of being needed in pretty much every project I do.
  • It's really not much code
  • We'd like to add support for template tag attributes, which we can't currently do.
  • We can submit PRs with tests if we know MDG will pull them.
@stubailo
Copy link
Contributor

Hey, this package looks like it has some pretty cool features. I'm curious what an app that uses template extensions heavily looks like from an architectural point of view.

Can you post an app that you've built using these features that you think is a good demonstration of how these patterns should be used?

@rclai
Copy link

rclai commented Feb 11, 2015

One of the biggest things that the package offers is the dynamic parent data fetching. Currently we're always needing to guess which level our parent data is in, which is really annoying. It also becomes a maintenance nightmare because if we decide to change the template in any way, our current parent data calls will break and we would need tor recalculate the correct level. Eric's template-extension would do away with the parent data maintenance nightmare in a set and forget manner.

@rclai
Copy link

rclai commented Feb 11, 2015

I think the first bullet point has been addressed right?

@aldeed
Copy link
Contributor Author

aldeed commented Feb 17, 2015

@stubailo, it's a mix of features useful for various reasons.

  1. Global callbacks are useful for jquery init on every template. There's one example courtesy of @SachaG here, but I have also used for i18n purposes.
  2. Template.forEach helps loop through all templates, which is not easy to do due to the strangeness of the Template object. This can be useful, for example, if you want to make a package that automatically finds templates with a certain prefix or suffix.
  3. hooks method is not as important since the next Meteor release has support for multiple callbacks, but I would contend that hooks API is nicer since it aligns with events and helpers.
  4. replaces is very useful when relying on a package's templates, but you need to alter it a bit without losing all the events and helpers provided by the package. There are several apps and packages that actively recommend this as the correct way to customize the templates they provide. copyAs is similar but gives you a custom copy while still having the original available.
  5. The inherits functions provide cleaner code when you have many similar templates, versus defining the function elsewhere and referencing it multiple times. Also helpful when extending and overriding templates provided by packages.
  6. template.get() is more reliable than specifying a certain parent level

@SachaG
Copy link
Contributor

SachaG commented Feb 17, 2015

Telescope doesn't use this package (it didn't exist yet at the time), but it does use the template replacement feature for every single template.

This makes it possible for people to easily replace any template of the app right from a package, which means they don't have to touch the core codebase. So it would be awesome to have this as a core Meteor feature!

@rclai
Copy link

rclai commented Feb 17, 2015

There are other pretty cool Meteor-centric packages that were created thanks to template-extensions that I mentioned in the Blaze 2 Hackpad:
https://atmospherejs.com/chriswessels/hammer
https://atmospherejs.com/mpowaga/template-styles

@aaronjudd
Copy link

@stubailo We're using this very heavily for reaction commerce (in fact, born out of discussions @aldeed and I had about how to make package templates easily extendable at the app level). Certainly gets my vote for a core feature. One note-able use case, that is applicable to most meteor apps, is how easy this makes it to modify accounts template behavior, without actually replacing the accounts packages, or building new ones.

@stubailo
Copy link
Contributor

@aldeed, @benjamn and I looked at your package in detail and it looks like it solves a lot of real problems for people. We're interested in supporting some of these features officially, but it will take time to think about the best way to do it. We have already added at least one of the features - the new syntax for onCreated etc. in 1.0.4 allows you to register multiple callbacks on templates.

In the meantime, are there any missing features in Blaze that would make it a lot easier to implement your package that we could add?

@aldeed
Copy link
Contributor Author

aldeed commented Mar 17, 2015

I think the multiple callback support was actually the area that felt the most hacky, and I'm planning to update the pkg to use the new 1.0.4 hooks mgmt with fallback to the internal hook references, so that is pretty well solved.

One in particular that would be good to be moved to official code is Template.forEach. There's the weird checks for "body" template, and it feels like code that is too reliant on stuff the might change periodically. Maybe I'm just being paranoid, though. :)

As far as the replace/inherit/copy stuff, it's not too bad as a separate pkg. Mostly it's just relying on the underscore prefixed properties that worries me. Really I just wanted to be sure that these use cases are being considered when planning the overall direction of blaze/templating pkgs. However you want to incorporate the ideas is fine.

cc @mitar @grabbou who contributed some of the functions.

@aldeed
Copy link
Contributor Author

aldeed commented Mar 17, 2015

Oh, the one other useful change would be if template tag parser allowed extra attributes besides name and attached them to the Template object for use in code. Then the pkg could support a syntax like <template name="foo" inherits-helpers-from="bar">

@SachaG
Copy link
Contributor

SachaG commented Mar 17, 2015

Oh that'd be pretty awesome!

@mitar
Copy link
Contributor

mitar commented Mar 17, 2015

+1 for attributes.

@stubailo
Copy link
Contributor

@aldeed ooh, that's a good idea - although I wonder if there will be a proliferation of attributes if we add that feature. Is Template.foo.inheritFrom("bar") not sufficient?

@aldeed
Copy link
Contributor Author

aldeed commented Mar 18, 2015

@stubailo the current way is sufficient, just not as slick imo. In some cases ("replaces" being the most likely one), you often don't need any js, so it would allow everything to be done in the html file. All in all it's not a big difference, though. Not sure about proliferation of atts.. could be true.

@stubailo
Copy link
Contributor

I think being able to have different attributes is an awesome idea if done right. Can someone submit an FR for it?

@aldeed
Copy link
Contributor Author

aldeed commented Mar 20, 2015

FR #3980

@0o-de-lally
Copy link

+1 for @aldeed's implementation of Template.onRendered() , a parent for all templates to inherit from.

@hwillson
Copy link
Contributor

Migrated over to meteor/blaze#190 - closing here. Thanks!

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

No branches or pull requests

8 participants