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

Fix typo in object assign for replyOptions #34

Merged
merged 1 commit into from
Feb 12, 2019

Conversation

thejones
Copy link
Contributor

I believe this will fix my issue here: #33

As far as I can tell this just breaks because the wrong variable is used in the Object.assign let me know if this seems valid. Tests passed locally for me.

If you need any changes let me know.

@mcollina
Copy link
Member

mcollina commented Jan 10, 2019 via email

@thejones
Copy link
Contributor Author

@mcollina the more I look at this the more I realize I do not actually understand one essential part. I will happily add some tests but from a usage perspective can you please assist with the following? Note I am still new to Fastify so I am also getting up to speed on some things.

In fastify-reply-from the example (https://github.com/fastify/fastify-reply-from/blob/master/test/transform-body.js#L25-L36) shows this pattern

instance.get('/', (request, reply) => {
  reply.from(`http://localhost:${target.address().port}`, {
    onResponse: (res) => {
      reply.send(res.pipe(new Transform({
        transform: function (chunk, enc, cb) {
          this.push(chunk.toString().toUpperCase())
          cb()
        }
      })))
    }
  })
})

This is very clear to me as the (request, reply) is available and I have access to reply.send(...)

My main issue is I am not sure how to do the same with fastify-http-proxy

In this case I have the follwing code:

fastify.register(proxy, {
    upstream: 'myUrl',
    prefix: '/apiProxy',
    async beforeHandler(request, reply) {
      console.log('beforeHandler from proxy called'); // works as expected
    },
    replyOptions: {
      rewriteHeaders: headers => Object.assign({ 'x-from-proxy': 'yes-indeed' }, headers), // works as expected
      onResponse: (res) => {
        // At this point in the code how to I send the reformatted/modified response? What has access to the reply object?
      },
    },
  });

As far as i can tell reply-from just streams the res back to me (https://github.com/fastify/fastify-reply-from/blob/master/index.js#L103) I just need to know where I can get access to a reply object or how I would actually send the res after I do my transformation.

Thanks again for the help.

@mcollina
Copy link
Member

You'll need to wrap that function so that req and reply are available.

fastify.register(proxy, {
    upstream: 'myUrl',
    prefix: '/apiProxy',
    async beforeHandler(request, reply) {
      console.log('beforeHandler from proxy called'); // works as expected
    },
    replyOptions: {
      rewriteHeaders: headers => Object.assign({ 'x-from-proxy': 'yes-indeed' }, headers), // works as expected
      onResponse: (req, reply, stream) => {
        reply.send(stream)
      },
    },
  });

@thejones
Copy link
Contributor Author

@mcollina apologies on this. I tried that out and tried hacking in a few things here and there to fastify-reply-from but I did not have any luck. If you have any other tips I would be very grateful.

@mcollina
Copy link
Member

Something like this should do the trick:

  test('passes onResponse option to reply.from()', async (t) => {
    const proxyServer = Fastify()

    proxyServer.register(proxy, {
      upstream: `http://localhost:${origin.server.address().port}`,
      prefix: '/api',
      replyOptions: {
        onResponse (reply) {
             reply.send({ something: 'else' })
          }
      }
    })

    await proxyServer.listen(0)

    t.tearDown(() => {
      proxyServer.close()
    })

    const { body } = await got(`http://localhost:${proxyServer.server.address().port}/api`)
    t.match(body, { something: 'else' })
  })

@mcollina
Copy link
Member

fastify-reply-from was updated in fastify/fastify-reply-from#43

@thejones
Copy link
Contributor Author

@mcollina I think this is updated with all of the latest and the new test. Running it through Standard on my machine made a bunch of changes that I can change back if needed. 🤷‍♂️

Thanks again.

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.

LGTM

@mcollina mcollina merged commit efd2593 into fastify:master Feb 12, 2019
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