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

wrap consolidate.js into a render function #100

Closed
doodzik opened this issue Jun 3, 2016 · 6 comments
Closed

wrap consolidate.js into a render function #100

doodzik opened this issue Jun 3, 2016 · 6 comments
Assignees

Comments

@doodzik
Copy link
Contributor

doodzik commented Jun 3, 2016

This should be done so we can exchange our render engine easier.
Which in turn can let us ease the way to support multiple render engines.

for reference #96

the consolidate.requires can be handled in a custom render function. So there is no need to expose it anymore.

@ismay
Copy link
Contributor

ismay commented Jun 10, 2016

I like the idea of metalsmith-layouts being agnostic to the library that does the rendering, but after thinking about I don't think it's doable for this lib. It needs a consistent api, and so far it's already enough of a challenge doing that for consolidate, let alone multiple rendering engines.

If we're going to refactor, I think it would be good to focus on supporting a single engine, and not try to support everything. That'll probably make things to cluttered and prone to breakage. Also, after looking at jstransformer, apparently they have a transformer for consolidate :). So we could just support jstransformer, and offload render engine support to jstransformer. That way, we can always extend support by adding a jstransformer and our api remains the same.

I like the modular approach that jstransformers use. Should be very maintainable, and seems like the modern way to go. That being said, since https://github.com/RobLoach/metalsmith-jstransformer exists, I don't know how necessary it is that we switch to jstransformers as well.

@RobLoach
Copy link

As I mentioned in kalamuna/metalsmith-jstransformer#9 , the easiest way to switch would be conslidate-jstransformer.

There are a few things metalsmith-jstransformer does on top of metalsmith-in-place/layouts. Worth a read over at kalamuna/metalsmith-jstransformer#9 . Again, an easy way to try JSTransformers out would be replacing Consolidate.js with conslidate-jstransformer .

@doodzik
Copy link
Contributor Author

doodzik commented Jun 20, 2016

I think it would be a good choice to support only one engine, too. We wouldn't achieve being agnostic to the library that does the rendering anyway.
But I would be in favour of refactoring the render related logic into its own function. This function should be overwritable, so people can use their own logic.

@ismay
Copy link
Contributor

ismay commented Jun 26, 2016

Given the intended switch to jstransformers I think we can close this. What do you think @doodzik

This function should be overwritable, so people can use their own logic.

We could give this a try, see if the rendering can be isolated well enough for people to modify the logic without creating an entirely new plugin.

@doodzik
Copy link
Contributor Author

doodzik commented Jun 26, 2016

I would still want to keep this one open, because that are two different issues.

... without creating an entirely new plugin.

I have a vague idea on how to accomplish that. If you don't mind I'll give it a try and we discuss it later.

@ismay
Copy link
Contributor

ismay commented Jun 27, 2016

Ok yeah, no problem! 👍

@doodzik doodzik self-assigned this Jun 27, 2016
@doodzik doodzik closed this as completed Sep 25, 2016
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

No branches or pull requests

3 participants