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

server swagger-ui using the swagger-ui-dist module #37

Closed
wants to merge 1 commit into from
Closed

server swagger-ui using the swagger-ui-dist module #37

wants to merge 1 commit into from

Conversation

PavelPolyakov
Copy link
Contributor

@PavelPolyakov PavelPolyakov commented Mar 23, 2018

I've checked that in the old (current) version there is a bug regarding oAuth.
Probably it was already solved in the original swagger-ui/dist.

Regarding the permissions - let me know if I should put them back. Since I don't see any reason to have them 0755 and 0644 are from the original repo.

@mcollina
Copy link
Member

How did you update swagger-ui? Can we use https://www.npmjs.com/package/swagger-ui-dist instead?

@PavelPolyakov
Copy link
Contributor Author

PavelPolyakov commented Mar 23, 2018

@mcollina
I just copied over the files from the current master of the swagger-ui.
Actually in the current state of PR I forgot to serve new png files.

Yes, we can use dist as dependency, kind of I do it here:
https://github.com/PavelPolyakov/fastify-serve-swagger-ui/blob/master/src/index.js#L58

also it's possible to serve everything through fastify-static.

would you support integration with static + dist ?

@mcollina
Copy link
Member

mcollina commented Mar 23, 2018 via email

@PavelPolyakov
Copy link
Contributor Author

done as static + dist.

in my previous PR there was a bug for redirecting /documentation to /documentation/ when being served by the reverse proxy. So if you were calling /upstream/documentation it was redirecting you to /documentation/. Now should be fixed, tested with fastify-http-proxy.

since we are not storing static files anymore, I've removed the assertions which were testing the content.

this hook is a little bit strange as for me, since it will really listen for the all incoming requests. Let me know if you think there is better solution.

@PavelPolyakov PavelPolyakov changed the title update of the swagger-ui files server swagger-ui using the swagger-ui-dist module Mar 23, 2018
request.raw.originalUrl === `/documentation/index.html`
) {
reply.header('Content-Type', 'text/html; charset=UTF-8')
payload = files.index
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should not be needed, maybe fastify-static has a bug. Why do you need it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is needed since I do not change the file from the original and content of swagger-ui-dist remains the same. But I hijack the request to the root or index.html and send our custom file instead of the default index.html.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think these prefixes are correct. I can mount this plugin with a given /swagger and it will add the ui as /swagger/documentation/.

A better approach would be to either have a post-install script or a prepublish  script where we do these changes and replacements. A prepublish might be better, because we can't guarantee that regexps are stable across versions. Why do we need those changes anyway?

Copy link
Contributor Author

@PavelPolyakov PavelPolyakov Mar 25, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This changes needed because by default it index.html of the swagger ui leads to the pet store.
https://github.com/swagger-api/swagger-ui/blob/master/dist/index.html

I can mount this plugin with a given /swagger and it will add the ui as /swagger/documentation/.

how would you do that? I think prefix option is not supported.

I've tried this together with example/dynamic.js:

fastify.register(require('../index'), {
  swagger: {
    info: {
      title: 'Test swagger',
      description: 'testing the fastify swagger api',
      version: '0.1.0'
    },
    host: 'localhost',
    schemes: ['http'],
    consumes: ['application/json'],
    produces: ['application/json']
  },
  exposeRoute: true,
  prefix: '/swagger'
})

the documentation appeared on localhost:3000/documentation still.

a prepublish script where we do these changes and replacements.

you mean on prepublish we copy everything into our static folder and then serve it instead of the original swagger-ui-dist?

A prepublish might be better, because we can't guarantee that regexps are stable across versions.

Yes, the original html can be changed, but since the version is fixed, I think it's safe to rely on the regex.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is the new variant #39

@PavelPolyakov
Copy link
Contributor Author

closing since the other implementation won

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

Successfully merging this pull request may close these issues.

2 participants