allow themes to copy assets to the output directory #387
Conversation
bin/raml2html
Outdated
| @@ -58,17 +57,7 @@ if (argv.template) { | |||
|
|
|||
| // Start the rendering process | |||
| raml2html | |||
| .render(input, config, argv) | |||
There was a problem hiding this comment.
Thanks for your PR! Looks like a very nice change, and a nice way to handle themes that require the output path to be set (rather then outputting to the console).
One comment I have: would it be possible to keep the .render(input, config, argv) line here, followed by the new handleOutput logic? I feel that the render call should not be within handleOutput. I'd also rename handleOutput to simply write. So we have render followed by write.
There was a problem hiding this comment.
Now that I look at it some more, I am wondering: how does a theme set this config.copyAssets flag? Combined with my other question about that export, I think there might be another way to go to implement this behaviour: do something similar as processRamlObj and postProcessHtml, where themes can override those functions but we supply a default one. Then a theme can just do whatever it wants, throw an error when needed, etc.
There was a problem hiding this comment.
i’ve started with the handleOutput call because i wanted it to throw early if the theme wants to store assets but the user didn’t choose to provide the --output option. otherwise the html would have been rendered for nothing. however i could move this check to the bin script.
concerning your second comment: I’m a bit confused. In index.js#L128 the theme config is loaded. In the theme I’m working on I’ve defined the main property in the package.json file to my theme config and I’m exporting an object there that defines processRamlObj and (with this PR) copyAssets. Isn’t that how it’s supposed to be working?
There was a problem hiding this comment.
Yes, you are correct. I always see the theme as an object that has a bunch of methods on it, but you're right that it can of course just contain properties. However, this implementation is still maybe not flexible enough. What if some other theme wants to write to an S3 bucket for example? Now raml2html "owns" the new handleOutput, we have to maintain it, and if other themes have other wishes, we have to keep adding new options to it. By letting theme supply the entire write function itself instead of only supplying 2 config parameters, the theme has 100% flexibility and raml2html doesn't need to own that complexity.
There was a problem hiding this comment.
ah… now I get it. I’ll update the PR :). is write the name you’d prefer for this config option?
There was a problem hiding this comment.
let's go with writeOutput
index.js
Outdated
| @@ -153,6 +181,7 @@ function getConfigForTheme(theme, programArguments) { | |||
| module.exports = { | |||
| getConfigForTemplate, | |||
| getConfigForTheme, | |||
| handleOutput, | |||
There was a problem hiding this comment.
Why is this getting exported? It doesn't seem like this needs to be public API, as the behaviour is controlled by a config flag? It's not like a theme needs to override this function?
There was a problem hiding this comment.
i didn’t think of raml2html as an api but more of an application. so i never thought that it would become part of an api :). uhm. the reason i moved it to the index.js file is that i didn’t want to bloat up the bin script, as I usually try to isolate cli code from the code that actually does things.
There was a problem hiding this comment.
Right but now we have themes that have to set options (somehow) that control the output functionality, contained within the main script (instead of the cli script). I think my suggestion to have themes supply their own writeOutput function, ala the way postProcessHtml works basically, is probably the best way to go. It leaves all logic in one script, all called in the same way, and doesn't need extra config options since the themes just supply their own write function.
Themes may override the default output implementation by definining the `writeOutput` function as part of their config. `writeOutput` receives the rendered html, the config and the cli options as arguments and may implement some form of asset generation or custom write targets.
|
sorry for the delay. here are the requested changes :) |
|
It's merged to develop but not released quite yet. I'd love for someone to confirm that all existing use cases work as expected :) |
|
@kmohrf I've created a ticket to write documentation for theme authors (https://github.com/raml2html/raml2html/issues/388). As you've noticed yourself, it's something that's currently lacking. If you feel like taking a stab at it, feel free to make a start! :) |
|
@kevinrenskers I’m about to publish a theme myself next week. If I find some time for documentation afterwards I’ll gladly contribute to the docs :) |
|
just fyi @kevinrenskers I just published raml2html-werk-theme that uses this feature. There is still a glitch (unrelated to this PR) that might be a sidenote for #388, because right now the theme only works, if I use a filesystem reference for the |
|
For |
|
I've published 7.1.0 btw. |
|
Note that this change re-introduced #259 when the output is stdout: process.stdout.write(result, resolve);rather than process.stdout.write(result);
resolve(); |
|
You are completely correct, thanks for pointing it out. Fixed and published. |
Hey,
this PR adds support for themes to add their own assets to the output directory of the html file. The primary reason for adding this feature is that I need a theme that bundles its assets in order to provide api documentation on local networks that don’t have access to the internet.
Themes may define a
copyAssetsmethod as part of their config. If they do the function will receive the target directory of the output file as their only argument and should return a Promise to indicate completion. I thought about adding some logic to handle things like strings as arguments for thecopyAssetsconfig option, but that is likely to fall short in terms of what people might want to do with this. The theme should know best how to handle its assets.I wasn’t quite sure where to add the documentation for this. If there is a theme guide somewhere I'll be happy to submit a pull request.
Thank you for your time and your work you put into this project!