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

Api fastify #21

Merged
merged 7 commits into from
Mar 16, 2020
Merged

Api fastify #21

merged 7 commits into from
Mar 16, 2020

Conversation

deluxor
Copy link
Contributor

@deluxor deluxor commented Mar 16, 2020

I took the initiative to make a major refactor to the project in order to facilitate infrastructure developments and support.
Please check the README.md

Copy link
Contributor

@hugoduraes hugoduraes left a comment

Choose a reason for hiding this comment

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

Please don't delete the following files, as they are useful:

  • .editorconfig - for keeping consistency between different editors/IDEs
  • .nvmrc - for the ones using NVM (me, at least), this file is useful for loading the correct version of Node
  • api.apib - are the specs moving somewhere else?

@@ -16,7 +16,7 @@ jobs:

strategy:
matrix:
node-version: [12.x]
node-version: [13.x]
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should stick with an LTS version of Node.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are some specific features only supported on 13.x, for example async hooks, and they are really important in logging, which in this case i didn't have implemented yet.

Copy link
Contributor

Choose a reason for hiding this comment

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

Async hooks are still in experimental state for both 12.x and 13.x! Don't thin we should rely on them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well it was closed on nodejs/node#31945 and released upstream in 13.10.0 https://github.com/nodejs/node/blob/master/doc/changelogs/CHANGELOG_V13.md#13.10.0
Need to wait for documentation to update.

unix_ts: {
type: DataTypes.INTEGER,
allowNull: true,
defaultValue: 'date_part(epoch'
Copy link
Contributor

Choose a reason for hiding this comment

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

You missed the ending parenthesis

timestamp: {
type: DataTypes.DATE,
allowNull: true,
defaultValue: 'timezone(utc'
Copy link
Contributor

Choose a reason for hiding this comment

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

You missed the ending parenthesis

timestamp: {
type: DataTypes.DATE,
allowNull: true,
defaultValue: 'timezone(utc'
Copy link
Contributor

Choose a reason for hiding this comment

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

You missed the ending parenthesis

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 seems to be a glitch on the generator from sequelize.

unix_ts: {
type: DataTypes.INTEGER,
allowNull: true,
defaultValue: 'date_part(epoch'
Copy link
Contributor

Choose a reason for hiding this comment

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

You missed the ending parenthesis

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 just picked the schema from the git repo and placed it on the project

CREATE UNIQUE INDEX IF NOT EXISTS user_status_id0 ON user_status (id);
DELETE FROM user_status;
INSERT INTO user_status(status) values('normal');
INSERT INTO user_status(status) values('self quarentine');
Copy link
Contributor

Choose a reason for hiding this comment

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

typo, it should be "quarantine"

DELETE FROM user_status;
INSERT INTO user_status(status) values('normal');
INSERT INTO user_status(status) values('self quarentine');
INSERT INTO user_status(status) values('quarentine');
Copy link
Contributor

Choose a reason for hiding this comment

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

same as above

package.json Outdated
"start": "node server.js",
"start:dev": "nodemon server.js"
"start": "node app.js",
"dev": "nodemon -w debug -P app.js",
Copy link
Contributor

@hugoduraes hugoduraes Mar 16, 2020

Choose a reason for hiding this comment

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

Nodemon is missing on the dev dependencies.

Also, can you add $NODE_DEBUG_OPTION env var to the dev script:

nodemon $NODE_DEBUG_OPTION -w debug -P app.js

It's useful for debugging for the ones using JetBrains IDEs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure why not?


fastify.post('/case', { schema: { body: fastify.schemas().createCase } }, async (request, reply) => {
try {
const { postalCode, geo, condition, timestamp, symptoms } = request.body
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the request.body automatically validated by specifying the schema?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, it is.

@deluxor deluxor merged commit c454511 into master Mar 16, 2020
@deluxor deluxor deleted the api-fastify branch March 16, 2020 14:19
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