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

serve swagger-ui through the swagger-ui-dist; v2 #39

Merged
merged 1 commit into from
Mar 28, 2018
Merged

serve swagger-ui through the swagger-ui-dist; v2 #39

merged 1 commit into from
Mar 28, 2018

Conversation

PavelPolyakov
Copy link
Contributor

this is another variant of #37 .
preparing the static folder before publishing the module (during the development needs to be done manually)

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

Good work!

tap-snapshots
test
.travis.yml
var
Copy link
Member

Choose a reason for hiding this comment

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

Can you remove this file?

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.

yes, I can, but, I wanted static to be ignored by git while be included in the npm. I see it's only possible with adding .npmignore.

plus I think that it's not really needed to include tests and other stuff into the package itself.

what do you think?

Copy link
Member

Choose a reason for hiding this comment

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

Oh right. Ok.

package.json Outdated
@@ -4,7 +4,9 @@
"description": "Generate Swagger files automatically for Fastify.",
"main": "index.js",
"scripts": {
"test": "standard && tap test/*.js"
"prepare:swagger-ui": "node var/npm/prepare-swagger-ui",
Copy link
Member

Choose a reason for hiding this comment

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

can you use prepublish  instead? https://docs.npmjs.com/misc/scripts.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

routes.js Outdated
method: 'GET',
schema: { hide: true },
handler: sendStaticFiles
// server swagger-ui with the help of fastify-static
Copy link
Member

Choose a reason for hiding this comment

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

typo, it should be "serve swagger-ui with the help of fastify-static".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@@ -43,28 +28,15 @@ function fastifySwagger (fastify, opts, next) {
url: '/documentation',
method: 'GET',
schema: { hide: true },
handler: (request, reply) => reply.redirect(request.raw.url + '/')
handler: (request, reply) => reply.redirect('./documentation/')
Copy link
Member

Choose a reason for hiding this comment

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

Why the dot in './documentation/'?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so my case was the next, I serve my service under the reverse-proxy, like:
/service points to the service itself, let's say localhost:3000.

With the previous version, when I was visiting /service/documentation it was redirecting me to the /documentation/. With the current version it works fine.

Here how I've checked it:

'use strict'

const fastify = require('fastify')()
const proxy = require('fastify-http-proxy')

fastify.register(require('../index'), {
  mode: 'static',
  specification: {
    path: './examples/example-static-specification.yaml'
  },
  exposeRoute: true
})

fastify.register(proxy, {
  upstream: 'http://localhost:3000/',
  prefix: '/upstream/' // optional
})

fastify.listen(3000, err => {
  if (err) throw err
  console.log('listening')
})

(put to the script into examples, install fastify-http-proxy, open http://localhost:3000/upstream/documentation)

Copy link
Member

Choose a reason for hiding this comment

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

I think this is ok.

@@ -0,0 +1,39 @@
const fs = require('fs')
Copy link
Member

Choose a reason for hiding this comment

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

Just for curiosity, why create this folder structure and not a simple script in the root of the project?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like to have the root clean, since usually in the modern web project there are already a lot of dot files out there.

So usually I have:

  • src (for the code, which sometimes get compiled into the dist)
  • var (for all the various files such as build scripts, images for the readme, etc.)
  • maybe __tests__

And that's all.

Copy link
Member

Choose a reason for hiding this comment

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

Since it is just one file you can leave it inside the root folder, it is not our common practice use the var folder and this could lead to confusion :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

np, done

package.json Outdated
"test": "standard && tap test/*.js"
"prepare:swagger-ui": "node prepare-swagger-ui",
"test": "standard && tap test/*.js",
"prepublish": "npm run prepare:swagger-ui && npm test"
Copy link
Member

Choose a reason for hiding this comment

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

npm test is not needed here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed

package.json Outdated
},
"dependencies": {
"fastify-http-proxy": "^0.2.1",
Copy link
Member

Choose a reason for hiding this comment

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

I guess fastify-http-proxy should be in devDependencies.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sorry fastify-http-proxy should not be here at all, removed

@PavelPolyakov
Copy link
Contributor Author

can this be released? 🤗

Copy link
Member

@delvedor delvedor left a comment

Choose a reason for hiding this comment

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

LGTM

@mcollina mcollina merged commit 05ae846 into fastify:master Mar 28, 2018
@PavelPolyakov
Copy link
Contributor Author

Thanks!

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.

3 participants