Support static files references #137

kromol · 2018-12-18T10:38:00Z

No description provided.

mcollina · 2018-12-18T10:47:26Z

Thanks for your contribution!

Linting is failing:

standard: Use JavaScript Standard Style (https://standardjs.com)
standard: Run `standard --fix` to automatically fix some problems.
  /home/travis/build/fastify/fastify-swagger/routes.js:46:65: Extra semicolon.
  /home/travis/build/fastify/fastify-swagger/routes.js:49:31: Extra semicolon.
  /home/travis/build/fastify/fastify-swagger/routes.js:50:15: Extra semicolon.
  /home/travis/build/fastify/fastify-swagger/routes.js:55:41: Extra semicolon.
  /home/travis/build/fastify/fastify-swagger/static.js:54:42: Unexpected trailing comma.

Also, can you add a unit test?

kromol · 2018-12-18T10:59:49Z

Yep, I'll fix it. This is still work in progress. I just opened PR so you can track the progress from the beginning :)

kromol · 2018-12-20T09:42:03Z

@mcollina are you OK with using fastify@next for now and updating it to released version later?
Currently there are issues with router which are fixed in fastify 2.

cemremengu · 2018-12-20T09:51:10Z

@kromol I was waiting for this PR to land actually but if it is really necessary I will bump the version

mcollina · 2018-12-20T10:31:29Z

Currently there are issues with router which are fixed in fastify 2.

Which issue?
You can target https://github.com/fastify/fastify-swagger/tree/fastify-next, which is the update to the new version of fastify.

kromol · 2018-12-20T11:00:03Z

I meant this issue - fastify/fastify#1338

Currently the problem is that plugin will have to serve static files from two different locations:

Internal static directory with UI stuff
Directory with yaml/json documentation files <-- this should be dynamic

With current setup I initially was thinking about adding additional regex route to handle only yaml and json files and do not break other static stuff handled by fastify-static. Since it did not work I started to think about using fastify@next to overcome routing issues...

But I just realized that another way might be to put everything served by fastify-static under it's own prefix. It seems better option from my perspective since it separates UI stuff from other plugin endpoints and should work with current dependencies. I will give it a try today.

kromol · 2019-01-02T16:50:07Z

Happy New Year!
@mcollina pls review the PR when you get a chance

mcollina

You'd also need to add the new option to the type definition in https://github.com/fastify/fastify-swagger/blob/master/index.d.ts#L10 and a test for the typescript types.

mcollina · 2019-01-02T17:12:43Z

test/route.js

+    t.error(err)
+    t.strictEqual(res.statusCode, 404)
+  })
+})


Can you please add a test that checks that the Fastify not found handler is called? This is likely to fail/conflict on Fastify v2, and we need the test to make sure the behavior is consistent.

mcollina · 2019-01-02T17:17:17Z

routes.js

+        const contentType = ext === '.yaml' ? 'application/x-yaml' : 'application/json'
+        reply
+          .type(contentType)
+          .send(fs.readFileSync(filePath))


I think it's better if you switch to fastify-static reply.sendFile() method. You can just add the decorator by passing { serve: false } when registering fastify-static.

You'll need to register two instances of fastify-static, one with { serve: false } to use inside this function, and one with { decorateReply: false } to serve the swagger-ui. The two instances should not conflict/error.

Resolves issue fastify#134

…ileSync, added more tests, fixed docs, updated typings, ensure that custom NotFoundHandler is being called

kromol · 2019-01-03T13:02:52Z

Rebased my branch on the latest master, but did not squash commits yet for easier review

cemremengu · 2019-01-03T13:23:34Z

@kromol you dont need to squash

kromol · 2019-01-03T13:27:02Z

Ok, great. I just thought that it should eventually be one commit per issue to keep history clean.

README.md

cemremengu

LGTM

mcollina

LGTM

mcollina · 2019-01-03T14:28:47Z

routes.js

+    decorateReply: false
+  })
+
+  fastify.register(require('fastify-static'), {


Nit: You should probably move the require('fastify-static') at the top of the file in both instances.

Since we added prefix `/static` to separate swagger ui static files from other files we need to make sure that client wlll not use `/static` prefix for other calls (like for retrieving swagger.yaml)

kromol force-pushed the feature-#134/support-ref branch from 53683e2 to 5deeadf Compare December 21, 2018 11:38

kromol changed the title ~~WIP: Support static files references~~ Support static files references Dec 21, 2018

mcollina reviewed Jan 2, 2019

View reviewed changes

cemremengu mentioned this pull request Jan 2, 2019

Fixes for fastify v2 #130

Merged

kromol added 3 commits January 3, 2019 13:54

Add support for static files reference via $ref

602f7b0

Resolves issue fastify#134

Code review fixes: use fastify-static to serve files instead of readF…

dbccc03

…ileSync, added more tests, fixed docs, updated typings, ensure that custom NotFoundHandler is being called

Fixes after rebase

4ee6535

kromol force-pushed the feature-#134/support-ref branch from 5bd339e to 4ee6535 Compare January 3, 2019 13:01

cemremengu reviewed Jan 3, 2019

View reviewed changes

README.md Outdated Show resolved Hide resolved

README.md Outdated Show resolved Hide resolved

Code review fixes

91a8c12

cemremengu approved these changes Jan 3, 2019

View reviewed changes

mcollina approved these changes Jan 3, 2019

View reviewed changes

Single require for fastify-static

f9c011a

mcollina merged commit 43e4e85 into fastify:master Jan 4, 2019

kromol deleted the feature-#134/support-ref branch January 9, 2019 11:12

kromol mentioned this pull request Feb 12, 2019

External $ref does not work #134

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support static files references #137

Support static files references #137

kromol commented Dec 18, 2018

mcollina commented Dec 18, 2018

kromol commented Dec 18, 2018

kromol commented Dec 20, 2018

cemremengu commented Dec 20, 2018

mcollina commented Dec 20, 2018

kromol commented Dec 20, 2018

kromol commented Jan 2, 2019

mcollina left a comment

mcollina Jan 2, 2019

kromol Jan 3, 2019

mcollina Jan 2, 2019

kromol Jan 3, 2019

kromol commented Jan 3, 2019

cemremengu commented Jan 3, 2019

kromol commented Jan 3, 2019

cemremengu left a comment

mcollina left a comment

mcollina Jan 3, 2019 •

edited

kromol Jan 3, 2019

Support static files references #137

Support static files references #137

Conversation

kromol commented Dec 18, 2018

mcollina commented Dec 18, 2018

kromol commented Dec 18, 2018

kromol commented Dec 20, 2018

cemremengu commented Dec 20, 2018

mcollina commented Dec 20, 2018

kromol commented Dec 20, 2018

kromol commented Jan 2, 2019

mcollina left a comment

Choose a reason for hiding this comment

mcollina Jan 2, 2019

Choose a reason for hiding this comment

kromol Jan 3, 2019

Choose a reason for hiding this comment

mcollina Jan 2, 2019

Choose a reason for hiding this comment

kromol Jan 3, 2019

Choose a reason for hiding this comment

kromol commented Jan 3, 2019

cemremengu commented Jan 3, 2019

kromol commented Jan 3, 2019

cemremengu left a comment

Choose a reason for hiding this comment

mcollina left a comment

Choose a reason for hiding this comment

mcollina Jan 3, 2019 • edited

Choose a reason for hiding this comment

kromol Jan 3, 2019

Choose a reason for hiding this comment

mcollina Jan 3, 2019 •

edited