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

feat: move to compress and decompress route options and typescript improvements #199

Merged
merged 13 commits into from
Dec 1, 2021

Conversation

darkgl0w
Copy link
Member

@darkgl0w darkgl0w commented Nov 26, 2021

Hello.

Following #198 proposal, this PR aims to :

  • add compress and decompress route shorthand configuration properties
  • improve typescript types and actually test those types
  • update the documentation

Note: some of those changes are breaking changes and will require a major bump.

Checklist

index.d.ts Show resolved Hide resolved
onUnsupportedRequestEncoding?: (encoding: string, request: FastifyRequest, reply: FastifyReply) => Error | undefined | null;
requestEncodings?: EncodingToken[];
threshold?: number;
zlib?: unknown;
Copy link
Member Author

Choose a reason for hiding this comment

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

Previously it was defined as NodeModule, but I don't think it's ok, example :

const zlib = require('zlib')

app.register(fastifyCompress, {
  zlib: zlib // this will trigger a typescript argument error if set to NodeModule
})

unknown is more suited here I think. WDYT ?

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.

Could you avoid the breaking change? Just document the new way and copy the old option into the new one.

@darkgl0w
Copy link
Member Author

darkgl0w commented Nov 26, 2021

Could you avoid the breaking change? Just document the new way and copy the old option into the new one.

@mcollina > Yes code wise I think I can avoid the breaking change by applying your suggestion and by throwing a deprecation hint warning with typescript (Typescript wise it should be doable, I will try to research a way of tweaking routes config typescript types).
WDYT in the case where someone makes something like this ? Can I go and consider that the new shorthand provided values take precedence on the old ones ? Is it acceptable to you ?

'use strict'
const app: FastifyInstance = fastify()
app.register(require('fastify-compress'), { global: false })
app.get('/', {
  compress: {
    zlib: {
      createGzip: () => createCustomGzipOne() // This one will be the final chosen value. Is it OK ?
    }
  },
  config: {
    compress: {
      zlib: {
        createGzip: () => createCustomGzipTwo()
      }
    }
  }
}, (req, reply) => {
  // ...
})

@mcollina
Copy link
Member

The outer one takes precendence.

@darkgl0w
Copy link
Member Author

darkgl0w commented Nov 26, 2021

@mcollina > Oups sorry I made a mistake in my example, I intended to give precedence to the new compress option over the old { config: compress } one ?
Have I guessed correctly or is it the other way around ? :p

@darkgl0w darkgl0w changed the title feat!: move to compress and decompress route options and typescript improvements feat: move to compress and decompress route options and typescript improvements Nov 26, 2021
index.js Show resolved Hide resolved
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, this is ok

index.d.ts Show resolved Hide resolved
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 c5919b0 into fastify:master Dec 1, 2021
@darkgl0w darkgl0w deleted the feat-proposal branch December 1, 2021 14:08
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.

3 participants