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

Prepare fastify-accepts-serializer master for v2 #4

Merged
merged 12 commits into from
Nov 27, 2018
Merged

Prepare fastify-accepts-serializer master for v2 #4

merged 12 commits into from
Nov 27, 2018

Conversation

cemremengu
Copy link
Contributor

@cemremengu cemremengu commented Nov 18, 2018

This is for preparing master branch for v2.

I think there is still work to be done such as:

  • Removing boom
  • Using only tap for testing

but I it's better to include those in the real next branch I think once we merge this 😄

@coveralls
Copy link

coveralls commented Nov 18, 2018

Coverage Status

Coverage decreased (-3.9%) to 93.671% when pulling 3152cba on next into f802dda on master.

index.js Outdated Show resolved Hide resolved
.travis.yml Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@cemremengu
Copy link
Contributor Author

Not sure why coveralls decreases 50% 😄

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

@cemremengu
Copy link
Contributor Author

There might be a problem with tests here. I think they are not running properly.

@mcollina
Copy link
Member

cc @allevo?

@cemremengu
Copy link
Contributor Author

I think I found it, trying to fix. In plugin definition it begins with

fastify.register(require('fastify-accepts'), err => {

Looks like register doesnt have a callback any more?

reply.store.config.serializer also doesnt seem to exist anymore. Converting to reply.serializer

@mcollina
Copy link
Member

+1

@cemremengu
Copy link
Contributor Author

cemremengu commented Nov 18, 2018

What is the current equivalent this?

const config = {
  serializer: {
    serializers: [
      {
        regex: /^application\/yaml$/,
        serializer: body => 'my-custom-string'
      },
      {
        regex: /^application\/x-msgpack$/,
        serializer: body => 'my-custom-string-msgpack'
      }
    ]
  }
}

fastify.get('/request', { config }, function (req, reply) {
  reply.send({pippo: 'pluto'})
})

Currently only at the route level I guess like:

  fastify.get('/request2', function (req, reply) {
    reply
    .header('Content-Type', 'application/x-msgpack')
    .serializer(_ => 'my-custom-string-msgpack')
    .send({ pippo: 'pluto' })
  })

@cemremengu
Copy link
Contributor Author

cemremengu commented Nov 18, 2018

I seem to have fixed tests but they fail. There are 3 groups of tests and each pass on their own but not together. Couldn't figure out why they are interfering with each other 🤔

@cemremengu cemremengu changed the title Prepare master for v2 Prepare fastify-accepts-serializer master for v2 Nov 18, 2018
index.js Outdated
})
if (serializer) {
reply.type(type)
reply._serializer = serializer.serializeFunction
Copy link
Member

Choose a reason for hiding this comment

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

This work correctly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Strangely yes. You can take this one over if you want.

Copy link
Contributor Author

@cemremengu cemremengu Nov 18, 2018

Choose a reason for hiding this comment

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

Ok not strangely actually since this is still against fastify 1.x. This work is just for getting everything up to date and marking the transition.

nevermind this 😄

"unit": "mocha --check-leaks --full-trace --throw-deprecation --allow-uncaught test/*.js",
"test": "npm run lint && npm run unit",
"coverage": "nyc --reporter=lcov --reporter=html --check-coverage --lines 100 npm run unit",
"coveralls": "nyc report --reporter=text-lcov | coveralls"
Copy link
Member

Choose a reason for hiding this comment

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

this is needed by CI

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It can work without that actually. tap can automatically forward to coveralls.

https://github.com/cemremengu/fastify-oracle/blob/master/package.json#L6

Let me see if that works

@allevo
Copy link
Member

allevo commented Nov 18, 2018

I 've added "COVERALLS_REPO_TOKEN" env var during the build. So the coverage will be sent automatically to coveralls (I hope)

Copy link
Member

@allevo allevo left a comment

Choose a reason for hiding this comment

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

LGTM

Thanks a lot!!

@cemremengu
Copy link
Contributor Author

@allevo no problem it was a good challenge and practice, thanks for bearing with me! 😄

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

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.

None yet

4 participants