Skip to content

Express like syntax proposal#1

Merged
mcollina merged 9 commits intofastify:masterfrom
delvedor:express-like-syntax
Oct 7, 2016
Merged

Express like syntax proposal#1
mcollina merged 9 commits intofastify:masterfrom
delvedor:express-like-syntax

Conversation

@delvedor
Copy link
Member

@delvedor delvedor commented Oct 5, 2016

As titled.

The user has an express like API, while we have a more easy to manage options object.

@delvedor delvedor changed the title Expess like syntax proposal Express like syntax proposal Oct 5, 2016
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.

little nits and a missing unit test

const router = wayfarer()
const map = new Map()

beo.route = route
Copy link
Member

Choose a reason for hiding this comment

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

I will keep exposing this.

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 keep the test on for that?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure!

schema: schema,
handler: handler
})
}
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 add a unit test for post?

Copy link
Member

Choose a reason for hiding this comment

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

we should also add body-parser to read the body (assume JSON for now).

@delvedor
Copy link
Member Author

delvedor commented Oct 6, 2016

To support POST I've added a body parser, I chose body over body-parser because body does not change the req object.

I also re-exposed route and updated the tests.
Finally I've added a little error management .

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.

A couple of nits

beo.js Outdated
handleNode(handle, params, req, res, body)
})
} else {
throw new Error(`${req.method} method is not supported!`)
Copy link
Member

Choose a reason for hiding this comment

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

Don't throw. Return a 404.. and add a test about that :D

beo.js Outdated
jsonParser(req, function (err, body) {
if (err) throw err
handleNode(handle, params, req, res, body)
})
Copy link
Member

Choose a reason for hiding this comment

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

this function definition is hoisted, which means it will be allocated even if we do not use it. Move this body-parsing thing into its own function.


beo.post('/', schema, function (req, reply) {
reply(null, 200, req.body)
})
Copy link
Member

Choose a reason for hiding this comment

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

wrap this into its own test()

@mcollina mcollina merged commit 3ea1e5f into fastify:master Oct 7, 2016
@delvedor delvedor deleted the express-like-syntax branch October 8, 2016 06:47
delvedor pushed a commit that referenced this pull request Sep 18, 2017
jcaowml pushed a commit to jcaowml/fastify that referenced this pull request Dec 19, 2019
Electrode initial implementation
haggholm pushed a commit to haggholm/fastify that referenced this pull request Jun 12, 2021
franktip added a commit to ProfilingPromisesBenchmarks/fastify that referenced this pull request Aug 24, 2021
@chenyulun chenyulun mentioned this pull request Oct 22, 2021
2 tasks
@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 25, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants