Skip to content
This repository has been archived by the owner on Apr 25, 2019. It is now read-only.

Prepare for hapi v6.0 #69

Closed
hueniverse opened this issue May 27, 2014 · 15 comments
Closed

Prepare for hapi v6.0 #69

hueniverse opened this issue May 27, 2014 · 15 comments

Comments

@hueniverse
Copy link
Contributor

hapijs/hapi#1664

Quick review shows that moving plugin.loader(), requiring handlebars directly, and moving to absolute path to the templates directory will allow it to be backwards compatible change.

@Marsup
Copy link
Contributor

Marsup commented May 28, 2014

Are you deprecating helpersPath as well ?

I haven't been able to make it work with the advised changes, I'm on 5.1 and a structure like this worked better :

engines: {
  html: {
    module: require('handlebars')
  }
}

Still missing the helpers though, working on it.

@hueniverse
Copy link
Contributor Author

helpersPath should not change since that is always relative to the basePath or path. What's not working?

@Marsup
Copy link
Contributor

Marsup commented May 29, 2014

The helpers are unknown to handlebars, I've already checked the path is correct.
I've pushed my current setup in hapi6 branch if you want to give it a try.
What do you think of the need to put it under module ?

@hueniverse
Copy link
Contributor Author

You should not need to put it under module. The tests don't.

Can you construct a failing helpers test?

@Marsup
Copy link
Contributor

Marsup commented May 29, 2014

Well it's worse without module (Invalid view options [...] module is required), which is why I went for it.

The current tests already fail with missing helper as you can see on travis.

@hueniverse
Copy link
Contributor Author

I suspect it has something to do with you cloning the engine module multiple times. I'll look into it tomorrow.

@Marsup
Copy link
Contributor

Marsup commented May 29, 2014

I think I've found at least one cause of failure, the require on handlebars goes through Hoek.applyToDefaults which seems to break it, but I still need that module key even if I do the require directly.

@hueniverse
Copy link
Contributor Author

I forgot you are trying to be backwards compatible. Then yes, you need module in the views config.

@Marsup
Copy link
Contributor

Marsup commented May 29, 2014

Unless there's a compelling reason not to, I'll try to be, it's not a huge effort at least for that 6.0, I might revise that later if things change in hapi.

@Marsup
Copy link
Contributor

Marsup commented May 29, 2014

Is it normal that hapi gobbles the module part after providing it to plugin.views() ? Right after my original object becomes { html: { module: null } }.

@Marsup
Copy link
Contributor

Marsup commented May 29, 2014

Well, everything works now. Any thoughts on the changes ? v4.2.0...hapi6

@hueniverse
Copy link
Contributor Author

The entire way the options are setup mixing views and non-views settings is very messy. Why is the engine configurable when the paths to the templates are not? I don't understand what should and should not be customizable. Why isn't just the CSS customizable?

@hueniverse
Copy link
Contributor Author

See hapi6-eran branch.

@Marsup
Copy link
Contributor

Marsup commented Jun 1, 2014

I never bothered to change these options since it seemed fined when I took over the project, and people might want to override everything, from the rendering engine to css, so I've kept it that way.

This doesn't work with hapi 5 but I might as well go for hapi 6. I don't know if people follow hapi versions closely.

@hueniverse
Copy link
Contributor Author

What doesn't work? The tests are hapi-version-specific but the lib code should work on 6 and 5.

@Marsup Marsup closed this as completed Jun 13, 2014
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

2 participants