-
Notifications
You must be signed in to change notification settings - Fork 20
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
Add an 'offline' option to embed the spec in the rendered page #14
Conversation
@etene I'm sorry that I was gone soooo long. Your PR makes sense to me, and I'd like accept this. However, I have a bunch of suggestion of what to improve. Would you like to work on them, so we can land it asap? or you would prefer if I simply continue your work? |
Hey, thanks for answering, no problem about the delay !
I agree there's room for improvement, so I'd be glad to hear about your
suggestions and implement them.
… |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I've been away for couple days. I left some comments here, could you please take a look?
sphinxcontrib/redoc.py
Outdated
ctx['spec'] = os.path.join('_specs', specname) | ||
# "Offline" mode: the JSON spec is embedded in the page | ||
# TODO: only works for local files: add remote files download ? | ||
if ctx.get('offline') is True: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like offline
word in this context, perhaps we can choose a better word. What do you think about embed
or single-page
or embed_spec
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
embed
sounds better to me, I'll change it
sphinxcontrib/redoc.py
Outdated
# Parse & dump the spec to have it as properly formatted json | ||
with io.open(os.path.join(app.confdir, ctx['spec'])) as specfp: | ||
# Dumb heuristic | ||
parser_module = json if ctx['spec'].endswith("json") else yaml |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
YAML could be used to parse JSON. For the sake of simplicity, I'd go with YAML loading.
P.S: I mean, json
is usually much faster than YAML
in parsing, but this is not performance critical part. I mean end users wouldn't even notice that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't even know this was possible. Simpler, OK with me.
sphinxcontrib/redoc.j2
Outdated
</script> | ||
<script> | ||
var spec = JSON.parse(document.getElementById("spec").innerHTML) | ||
Redoc.init(spec); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think since we have this init
function here anyway, there's no much sense in passing redoc options via HTML attributes. Let's pass them via JavaScript instead. So on top level you will have only empty <redoc></redoc>
tag.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok !
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not a web developer so not really familiar with all that Javascript & HTML, but it seems from the docs that if we pass options to the init function, they must be in camelCase.
However sphinxcontrib-redoc's options are in "kebab-case" . I can't pass them directly this way.
Here's what I can do:
- Change sphinxcontrib-redoc's configuration options to camelCase (breaking change :-/ )
- Convert options from one form to the other (invisible for users but weird for future code readers)
What would you prefer ? Have I understood what you wanted correctly ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you understand it right. I was thinking about making uni-drectional map of kebab notation to camelCase. However, now I think it could be done in separate PR, so don't bother yourself. I think moving "spec" passing (either JSON or URL) to one place (ReDoc.init) is good enough for this PR. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great ! Just leave me some time to update the docs regarding this PR's feature, unless you want to do it, and I think it will be mergeable.
sphinxcontrib/redoc.py
Outdated
% (ctx['spec'], ver)) | ||
|
||
ctx['spec'] = json.dumps(spec_contents) | ||
else: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it makes sense to join this else
and if
below to reduce indentation level, like this:
# The 'spec' may contain either HTTP(s) link or filesystem path. In
# case of later we need to copy the spec into output directory, as
# otherwise it won't be available when the result is deployed.
elif not ctx['spec'].startswith(('http', 'https')):
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can do
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@etene thanks for working on this! I left couple stylistic comments. I believe we are really close to merge this, I'll wait for changes in docs. Once they are done, I'll gladly merge this.
sphinxcontrib/redoc.py
Outdated
@@ -30,10 +32,24 @@ def render(app): | |||
# relies on them. | |||
ctx.setdefault('opts', {}) | |||
|
|||
# "Embed" mode: the JSON spec is embedded in the page |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we change a comment into something like this? I think it might be helpful for readers to understand a rationale behind this chunk of code.
# In embed mode, we are going to embed the whole OpenAPI spec into
# produced HTML. The rationale is very simple: we want to produce
# browsable HTMLs ready to be used without any web server.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, yours is better, i'll change it
sphinxcontrib/redoc.py
Outdated
@@ -30,10 +32,24 @@ def render(app): | |||
# relies on them. | |||
ctx.setdefault('opts', {}) | |||
|
|||
# "Embed" mode: the JSON spec is embedded in the page | |||
# TODO: only works for local files: add remote files download ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personal opinion: no TODOs in code, they will never be fixed. :) If you think it's important to implement, could you please create a followup issue instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, I'll open an issue (and maybe do it if I have some time)
sphinxcontrib/redoc.py
Outdated
# TODO: only works for local files: add remote files download ? | ||
if ctx.get('embed') is True: | ||
# Parse & dump the spec to have it as properly formatted json | ||
with io.open(os.path.join(app.confdir, ctx['spec'])) as specfp: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, I believe we should explicitly set an encoding. If I'm not mistaken by default will be used encoding extracted from current locale, but OpenAPI spec may use another encoding.
So I can think about two options:
-
Explicitly use
UTF-8
and say that onlyUTF-8
specs are supported. At least we will know for sure which encoding to use to deal with files no matter which system we're running on. -
Use encoding from Sphinx config:
env.config.source_encoding
, and say that the specs must have the same encoding as other Sphinx documents. -
Read encoding value from per-spec configuration.
While third item is more flexible, it would require additional changes and thorough thinking. I'd suggest to go with either (1) or (2) in this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll implement (1) for now. Since the spec is generated by something that has nothing to do with Sphinx, I'd be reluctant to tie them together.
tests/test_integration.py
Outdated
] | ||
''')).render(opts=conf) | ||
redoc = {{ redoc }} | ||
''')).render(redoc=pprint.pformat([defaultconf])) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I maybe missed that, but what's the point in having pprint.pformat
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, it's probably something left over from testing, when I wanted the rendered template to be more human-readable. I'll remove it.
- use explicit encoding when opening specs (to be documented) - improve the comment for the embed option - remove useless debug pprint in tests
I think this should be about it, tell me if you have any more comments ! |
@etene Looks good! Thank you very much! |
Hello,
First of all thanks for the work you've put in this project, it has been really useful to me.
However, I desperately needed to be able to generate a completely static HTML page that doesn't load the spec externally, and:
file://
URLsredoc-cli
can do it, but requires a whole load of various Javascript libraries & isn't integrated into SphinxSee: Redocly/redoc#149
I figured embedding the JSON spec in a
<script>
element would be the way to go. I haven't altered the default behavior, just added anoffline
parameter.So what is does is, if the
offline
parameter is set toTrue
, parse the JSON or YAML spec and embed it in the rendered page. Doesn't work for remote URLs, but I could fix that if you're interested.Could you please consider merging ? Not yet though, I would like to update the docs first, if you're okay with it.
Thanks a lot !