Improved hogan.js support + engine-specific tests. #24

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
4 participants
Contributor

ovaillancourt commented Apr 20, 2012

  • Improved hogan.js support (now supports partial + caching).
  • Modifications to the tests, couple of quick fixes to the common test code + added support for custom per-engine test code. (Simply put a file named [engineName].js in the /test/engine folder and the tests it contains will automatically run along with the common tests for that same engine. See /test/engine/hogan.js for an example.)
Improvements to hogan.js support, adding partial support + caching.
Modifications to the tests, couple of quick fixes to the common test code + added support for custom per-engine test code. Simply put a file named [engineName].js in the /test/engine folder and the tests it contains will automatically run along with the common tests for that same engine. See /test/engine/hogan.js for an example.
+it('should support partials', function(done){
+ var path = 'test/fixtures/hogan/layout.hogan';
+ var locals = {
+ partials : { body: 'test/fixtures/hogan/body.hogan' }
@tj

tj Apr 20, 2012

Owner

hmm so this is how hogan partials work? or for "real" hogan are they html fragments?

Express has no notion of partials so if they were to be relative to ./views (or the "views" setting etc)
the user would have to be explicit

@ovaillancourt

ovaillancourt Apr 20, 2012

Contributor

For "real" hogan they're just fragments of text (or html if you're doing html stuff...) The part where you load the file isn't handled by hogan.

As for the partials part, I didn't take into account that they were to be relative to a certain path or something like that, just considered that people would have to input a correct path. Do you have any suggestion of modification? Something similar to the whisker's implementation is doable.

@ovaillancourt

ovaillancourt Apr 20, 2012

Contributor

Hm, and as another note, the whole {partials: {name : path}} construct is not the standard "hogan way" of providing partials.

Hogan instead has a 2nd argument where you normaly provide a hash of the compiled templates that are to be used as partials. The keys of the hash correspond to the names of the partial in the template...

@tj

tj Apr 25, 2012

Owner

hmm IMO we should try and stick mostly (for now) to the engines implementation so those docs are still relevant otherwise we'll have to document everything we do. Seems kinda annoying though as far as a template engine goes to have to pass some html fragments, but that's up to hogan I guess

@tj

tj May 9, 2012

Owner

turns out whiskers implements .__express for express 3.x, so its the same signature, no point duplicating that partial stuff I guess, doesnt have any docs for it though I dont think

@gsf

gsf May 9, 2012

Contributor

Yeah the only "docs" for whiskers.__express right now are in whiskers.js/test/server/test-express.js.

tarruda commented Jun 9, 2012

Hi

I have submitted an alternative implementation which will scan the template for partials/layout template references and load from the fs everything needed. See visionmedia#34.

Contributor

ovaillancourt commented Jun 9, 2012

Yeah, I'd recommend taking a look at the comments from this pull request since they apply to your implementation too.

Mostly the one from tj on avoiding to modify the template engine's expected behavior with how partials are included in it.

That being said, I've put this pull request on the ice. There's currently 3 active (and hanging) pull requests concerning partial support for mustache-based engines and pull request #29 is better than my own, so I'll probably not work on this anytime soon. I'm leaving it open but will most likely close it when #29 goes in, or merge #29 and generate a new pull request only for the tests.

tarruda commented Jun 10, 2012

The implementation I have submitted doesn't require any change on how to specify partials/layout templates, the user will just specify the template name and the required partials/layouts will be figured out. It follows these steps:

1 - pull the template source from the fs
2 - use the engine lexer to collect all referenced partials/layouts
3 - pull the sources for all the partials/layouts from the fs
4 - compile the template
5 - if options.cache == true, store the compiled template and its partials/layout
6 - render the template

Contributor

ovaillancourt commented Jun 10, 2012

Yes I do understand your pull request and I still think it's modifying the expect behaviour from hogan.

A Hogan user will expect to have to provide strings of text or precompiled templates as partials. It won't expect hogan to start loading files from the file system automatically based on the partial tag name - that's a change in hogan's behaviour. (Even thought you're not changing hogan itself, you're still changing how the engine behaves since your adapter also adds new behaviours and people have to interact with the engine through this adapter when using consolidate.)

That being said, there's been conflicting information as to what should be considered "correct" with regard to consolidate integration. It's been said in this pull that the consolidatejs implementations should stick as close as possible to their engine's standard behavior, but in pull request #29 it's said that forcing people to pass strings or html directly for partials suck. Soo basically nobody is following on the pull requests for lack of guidance.

@visionmedia -> we might want to just clear this whole hogan/mustache partial loading issue once and for good... it's a bit weird that express 3 users can't yet use partials without completely bypassing the view system...

Contributor

ovaillancourt commented Jun 10, 2012

@tarruda Just as a quick note though, I don't want you to feel that I'm attacking your actual implementation. I find it quite good and we're doing the same thing here at Keaton Row (where I work) - we just completely bypass the express view system and have a partial auto-loading system that scans the compile tree built on top of hogan exactly like you're proposing.

I'm just not quite sure that it fits within the consolidatejs mindset given the comments from this pull request and pull request #29.

tarruda commented Jun 10, 2012

@ovaillancourt No problem, now I understand what you meant by modifying the engine's normal behavior, but I don't think it is possible to provide an universal interface to all engines without behavior modification as some engines take into consideration filesystem loading while others(like hogan) leave that loading problem to the user.

Btw I have updated my commit, now it will consider partials/layouts referenced in other partials/layouts. Maybe the "asynchronous recursive template loading algorithm" can be refactored in order to replace current 'read' function and be reused by other engines.

Owner

tj commented Jun 12, 2012

you're right that we really need to provide the lowest barrier to entry as possible and make things nice to work with, but we also dont want to (ideally) have new documentation for each engine. I'll take a look

Owner

tj commented Jun 12, 2012

i cant remember what we decided to go with over-all haha, wasn't it filename relative partials? I'd be ok with it as long as it's documented in the readme and has good test coverage so it's not a maintenance burden

Contributor

ovaillancourt commented Jun 12, 2012

@visionmedia Yeah I think that was it. If my memory serves me well, it was what was implemented in pull #29.

Contributor

ovaillancourt commented Feb 22, 2017

Cleaning up old stale PRs - closing.

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