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

Improve codebase #134

Open
Eomm opened this issue Sep 19, 2019 · 4 comments
Open

Improve codebase #134

Eomm opened this issue Sep 19, 2019 · 4 comments
Labels
good first issue Good for newcomers

Comments

@Eomm
Copy link
Member

Eomm commented Sep 19, 2019

The actual options for getHeader is not compliant to the fastify codebase that support the chaining-call:

ex: this.header('Content-Type', 'text/html; charset=' + charset).send(html)

It would be awsome if @momofan or @florianb could work on this 💪

Mode info on how to implement this improvement:

point-of-view/index.js

Lines 52 to 53 in b7345f0

getHeader: () => {},
header: () => {},

They are replicating the core Reply methods and should behave accordingly. In fact, I'm not sure why the view decoration is on the server object. Views should be rendered in the context of a request, and in particular the context of sending a reply. So I would think the view decoration should be fastify.decorateReply('view', function () {}). Which would mean the following would be an outline of a solution:

fastify.decorateReply('view', function () {
  // other details omitted
  renderer.apply({getHeader, header}, args)

  function getHeader (key) {
    return this.getHeader(key)
  }

  function header (key, val) {
    return this.header(key, val)
  }
})

Originally posted by @jsumners in #129

@Eomm Eomm added the good first issue Good for newcomers label Sep 19, 2019
@florianb
Copy link

Thanks @Eomm - i can do it if @momofan doesn't want to take it.

@florianb
Copy link

It seems like @momofan won't pick it up - you might assign me @Eomm.

@weirdf0x
Copy link
Contributor

I would like to pick up this issue, if this is OK with you?

If I understood correctly, there are two issues at hand:

  • the extension should decorate the reply object, not the server (it would be a breaking change, wouldn't it?)
  • calls to the getHeader and header functions are not chainable, so they don't adhere to the way it is done in the framework and as for now have no implementation and just return empty objects

@mcollina
Copy link
Member

go for it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

4 participants