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

Automatically wrap route files #54

Closed
PatrickHeneise opened this issue Dec 5, 2019 · 8 comments · Fixed by #57
Closed

Automatically wrap route files #54

PatrickHeneise opened this issue Dec 5, 2019 · 8 comments · Fixed by #57

Comments

@PatrickHeneise
Copy link
Contributor

🚀 Feature Proposal

We're migrating an API to fastify and somehow the boilerplate

module.exports = function (fastify, opts, next) {
  fastify.route({
    method: 'GET',
    url: '/',
    handler
  })
}

feels a bit clunky. I'd like to propose just the route content in a file:

module.exports = {
  method: 'GET',
  url: '/',
  handler
}

For most cases this is totally sufficient. If additional config/options are required, a regular plugin/route function can be exported.

Motivation

As mentioned above, the plugin structure feels a bit clunky in each and every file.

Example

The default type is a function and the new proposal would be an object (method is always required anyway), so I'm thinking somewhere along the lines of (https://github.com/fastify/fastify-autoload/blob/master/index.js#L110):

const content = require(file)
let plugin

if (content && typeof content === 'object' && content.method) {
  plugin = function (fastify, opts, next) {
    fastify.route(content)
    next()
  }
} else {
  plugin = content
}

This is non-breaking, but to be safe it could also be flagged with an new option "wrapRoutes" or something like that.

I'm testing this locally, but I'm currently getting an error when running npm test?

test/error.js ......................................... 1/2
  not ok should match pattern provided
    found: >-
      Unexpected token '}' at
      /Users/patrick/Sites/fastify-autoload/test/error/lib/a.js:6
    pattern: '/Unexpected token \} at .*\/test\/error\/lib\/a.js:6/'
    at:
      line: 14
      column: 5
      file: test/error.js
    stack: |
      test/error.js:14:5
      Object._encapsulateThreeParam (node_modules/avvio/boot.js:408:13)
      Boot.callWithCbOrNextTick (node_modules/avvio/boot.js:339:5)
      release (node_modules/fastq/queue.js:127:16)
      Object.resume (node_modules/fastq/queue.js:61:7)
      node_modules/avvio/boot.js:155:20
      node_modules/avvio/plugin.js:178:7
      done (node_modules/avvio/plugin.js:136:5)
      check (node_modules/avvio/plugin.js:147:7)
    source: |
      t.match(err.message, /Unexpected token \} at .*\/test\/error\/lib\/a.js:6/)
@jsumners
Copy link
Member

jsumners commented Dec 5, 2019

You can already do what you desire:

const routes = fs.readdirSync('path/to/routes/dir/')
routes.forEach(r => fastify.route(require(r)))

The plugin pattern you show in your first example is merely the recommended method. It provided encapsulation and makes it easier to test individual routes.

@PatrickHeneise
Copy link
Contributor Author

PatrickHeneise commented Dec 5, 2019

Thanks! That's neat!

But that also means if we have an exception and need additional arguments, we'll have to handle that file outside the /path/to/routes folder as it can't be loaded that way?

Also the automatic folder-2-route prefixing is lost here :(

@jsumners
Copy link
Member

jsumners commented Dec 5, 2019

I thought this issue was on the core repo. I didn't realize it is in the autoload plugin repo. I know nothing of how this plugin works.

@PatrickHeneise
Copy link
Contributor Author

Thanks anyway! Right now we don‘t have any special cases, so this might do the trick, but in the long run I think it‘s better to solve this in the autoloader.

@mcollina
Copy link
Member

mcollina commented Dec 5, 2019

It would be great if you'd like to send a PR.

@PatrickHeneise
Copy link
Contributor Author

I will. As mentioned, I can't get the tests to pass on master at the moment and wonder what's going on there?

@mcollina
Copy link
Member

mcollina commented Dec 6, 2019

Can you open a seperate issue with the error you are getting?

@PatrickHeneise
Copy link
Contributor Author

Done. See #55

PatrickHeneise added a commit to PatrickHeneise/fastify-autoload that referenced this issue Dec 6, 2019
mcollina pushed a commit that referenced this issue Dec 6, 2019
* feat: automatically wrap full routes into fastify plugins. Closes #54

* add documentation in readme

* Update README.md

* more concise checks for object

* remove typeof

Co-Authored-By: James Sumners <[email protected]>
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 a pull request may close this issue.

3 participants