Skip to content

Conversation

@mcollina
Copy link
Member

@mcollina mcollina commented Oct 4, 2018

As title, I think the example could be simplified a lot.
This has the benefit to potentially avoid unhandled rejections

@mcollina mcollina requested a review from cemremengu October 4, 2018 09:44
Copy link
Contributor

@cemremengu cemremengu left a comment

Choose a reason for hiding this comment

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

LGTM this is much better, handles all errors in route

connectionString: 'postgres://postgres@localhost/postgres'
})

fastify.post('/user/:username', (req, reply) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

If we make this async, we can directly return return client.query right?

Copy link
Member Author

Choose a reason for hiding this comment

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

There is no need to use async at all because Fastify understand promises. However that won't be the normal case for applications.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah yes, thank you for the info!

@cemremengu
Copy link
Contributor

@mcollina FYI I can't merge this one

@mcollina
Copy link
Member Author

mcollina commented Oct 4, 2018

You can now, and you should be able to release as well :)

@cemremengu cemremengu merged commit 2994680 into master Oct 4, 2018
@cemremengu
Copy link
Contributor

Thank you! :) this will be a semver minor right?

@cemremengu cemremengu deleted the update-example branch October 4, 2018 10:41
@mcollina
Copy link
Member Author

mcollina commented Oct 4, 2018

patch!

@cemremengu
Copy link
Contributor

Ok so publishing only readme

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