Feature/fastify examples session #26
Conversation
Session storage three examples all localhost - simple http - simple https - redis based storage over https
update the docs with docker instructions.
…y-examples-session
markdown lint corrections A markdownlintignore file has been added as there will be multiple folders which contain node module readmes that do not comply.
There was a problem hiding this comment.
I tend to prefer to recommend the use of of https://github.com/fastify/fastify-cookie (with the signature) or https://github.com/fastify/fastify-secure-session.
servers/fastify/session/https-session-server-redis/https-session-server-redis.js
Show resolved
Hide resolved
servers/fastify/session/https-session-server/https-session-server.js
Outdated
Show resolved
Hide resolved
servers/fastify/session/https-session-server-redis/https-session-server-redis.js
Outdated
Show resolved
Hide resolved
take feedback and update as suggested.
alter the host name to take account of whether it is inside a docker container or not
servers/fastify/session/https-session-server/https-session-server.js
Outdated
Show resolved
Hide resolved
…ver.js Co-authored-by: Matteo Collina <[email protected]>
|
@mcollina ok, so there may be a misunderstanding by myself on this subject. Should all route handlers be async even if they are synchronous themselves. Is this a general practice? Or am I doing this correctly but you would rather see examples of asynchronous behavior with these session examples too? ( PS sincere thanks for reviewing ) |
I recommend this for examples because adding an asynchronous behavior is the normal case. There are significant differences between returning a value (or throwing) in an Starting from v3 this pattern is supported so everything works fine here, but it looks good only in non-production cases. |
|
ok, understood. I thought that was what you would say. Will make appropriate changes. |
add logging to examples where it was missing use async in all route handlers use consistent listen handler across examples
| @@ -6,6 +6,7 @@ const Fastify = require('fastify') | |||
| const fastifySession = require('fastify-session') | |||
| const fastifyCookie = require('fastify-cookie') | |||
|
|
|||
| const APP_PORT = 3000 | |||
There was a problem hiding this comment.
| const APP_PORT = 3000 | |
| const APP_PORT = process.env.PORT || 3000 |
use PORT from the environment first to decide where to listen.
Roll back to using a client for redis. @mcollina suggested I use fastify-redis. I think in the case where I am using fastify-session, I need to provide a store, and that store needs a redis client. As the fastify-redis uses a decorator it is not available at the time I register fastify-session and so using the client directly is the only way to do this. Please correct, still getting to grips with the framework, but others are liking it too.
servers/fastify/session/https-session-server-redis/https-session-server-redis.js
Outdated
Show resolved
Hide resolved
|
Then I shall try that. Live and Learn :) |
attempt multiple sessions to see what is happening.
markdown lint correction
| @@ -41,29 +34,38 @@ const fastify = Fastify({ | |||
| logger: true | |||
| }) | |||
|
|
|||
| fastify.register(fastifyCookie) | |||
| const initialization = async () => { | |||
There was a problem hiding this comment.
if you pass in the fastify app as a parameter, you can export this function and make this easily testable. I would also move the listen outside.
| const initialization = async () => { | |
| const initialization = async (app) => { |
There was a problem hiding this comment.
So a couple notes (again!):
The REAME shouldn't be in the vein of a tutorial - it should be more like a README if you were to publish this project as a standalone project on GitHub. Here are a few generic examples of the style I'm talking about. Definitely feel free to publish it as an article, though 👍🏻
Will test the docker-compose bits soon, but wanted to give this feedback ASAP.
|
cc @mhdawson is including an SSL cert here okay given the cryptography exports thing? |
|
oops didn't mean to close |
|
many thanks for the comments. I shall update the readme to follow the style suggested. The certificates are local host and are self signed. There are no cryptographic secrets within them they just allow for session to be tested on localhost using the secure setting. Otherwise the example is not representative. |
markdown correct openssl command missing 'o'
|
I have had a look at the example readme files you gave. Apart from the IBM example they are all installable modules and only the IBM readme is an example, so they don't really pertain to this type of examples repo. The IBM readme does in fact start off with in this tutorial. I am not really sure what is being asked for. |
|
Here's an example of a server from @Thiruppathi that aligns more with what's being asked for :) |
A slightly more advanced example starting with