Upgrade to Fastify v3 #85

thomheymann · 2020-03-23T09:27:33Z

This PR upgrades fastify-plugin to Fastify v3. The only changes were TypeScript related but I also aligned the filenames so that implementation and declaration files are consistent.

I have followed the new TypeScript guide here: https://github.com/fastify/fastify/pull/1966/files?short_path=17177d7#diff-17177d7eb9ef1a38f00590c051d2b873

However, I am not sure if the FastifyPlugin interface is correct as its requires knowledge of server, request and response types of the fastify instance at build time of the plugin. We don't know what the fastify instance looks like at build time though.

@Ethan-Arrowood - Can you have a look at the test/types.test.ts? I have added a test for this case so build currently fails

Example plugin and usage with new `FastifyPlugin` type

export const testPlugin: FastifyPlugin<{}> = fp(
  function (fastify, options, _next) {
    fastify.decorate('utility', () => options.hello)
  },
  '>=1'
);

const server = fastify()
server.register(testPlugin) // This works since `FastifyPlugin` defaults to HTTP

Example plugin with HTTP2 Fastify instance

const serverWithHttp2: FastifyInstance<Http2Server, Http2ServerRequest, Http2ServerResponse> = fastify({ http2: true });

serverWithHttp2.register(testPluginWithOptions) // This fails since `FastifyPlugin` defaulted to HTTP but Fastify instance is HTTP2

Yes, we could cast to unknown first and then to HTTP2 version of FastifyPlugin but I don't think that's a good type system:

serverWithHttp2.register((testPluginWithOptions as unknown) as FastifyPlugin<Options, Http2Server, Http2ServerRequest, Http2ServerResponse>) // 🙈

Might be missing something, let me know how you want this to work.

Checklist

run npm run test and npm run benchmark
tests and/or benchmarks are included
documentation is changed or added
commit message and code follows Code of conduct

mcollina · 2020-03-23T09:41:06Z

Would you mind to include those two examples in the README?

mcollina · 2020-03-23T09:42:00Z

Also, CI is failing everywhere.

Plus, can you drop Node 6 and 8? Thanks.

Ethan-Arrowood · 2020-03-23T14:01:10Z

@thomheymann are you aware that this work is already done in this PR here: #81?

I'm happy to let you contribute to that branch if you'd like to complete the PR but I believe I was waiting to publish it when the first v3 release candidate was released

thomheymann · 2020-03-23T15:36:43Z

Hi Ethan, I wasn't aware of the PR #81 But I think you're going to run into the same typing issues with HTTP2 Fastify instances.

Ethan-Arrowood · 2020-03-24T02:37:23Z

Alright cool I'm in favor of this pr then. I'll do a proper review now 😄

Ethan-Arrowood

good work so far. keep it up!

Ethan-Arrowood · 2020-03-24T02:40:36Z

test/types.test.ts

+server.register(testPluginWithAsync)
+
+// Register with HTTP2
+const serverWithHttp2: FastifyInstance<


You shouldn't need to specify these generics any longer. Let me know if removing them causes issues and I'll jump in and try and figure out whats up.

I don't need to specify the generics but if I remove them I get HTTP types, not HTTP2 types.

I think we might need to look at the types themselves.

When authoring a plugin, we will not know what generics will be passed into FastifyInstance so maybe the types need to cater for that?
e.g. FastifyPlugin<Server = http.Server & https.Server & http2.Http2Server>

The other issue I find strange is that server type is determined by fastify options. If https options are passed in, the server is of type https. If http2 boolean is passed in, the server is of that type. Shouldn't the types reflect that mechanism?

Not exactly, the server types are supposed to be inferred based on the options object. The generics only exist if you want to extend the types of those servers, request, or responses. See here:

https://github.com/fastify/fastify/blob/f1cfcadbe201f98137a5b2a9ee1159215b5d81e5/fastify.d.ts#L20-L44

This is how the fastify() method is defined in the next types (if you're curious its using discriminant unions and function overloads to achieve it).

Fastify-Plugin should forward the generic properties through to the underlying instance, and it should also enable the user to omit them if they are not decorating those generic parameters

You're right, the server type is correctly inferred, that's great!

I'm still not clear how the FastifyPlugin type is supposed to be used though.

Plugins are defined like so:

export const testPlugin: FastifyPlugin = fp( function (fastify, _options, _next) { console.log(fastify) // Is now of type `FastifyInstance<Server, IncomingMessage, ServerResponse, FastifyLoggerOptions<Server, IncomingMessage, ServerResponse>>` }, {}, )

How will TypeScript know what options have been passed into fastify? The fastify instance hasn't even been created at that point.

Consequently in user land, the following line is failing because of the defaults used by FastifyPlugin type:

const serverWithHttp2 = fastify({ http2: true }); serverWithHttp2.register(testPlugin) // Argument of type 'FastifyPlugin<{}, Server, IncomingMessage, ServerResponse>' is not assignable to parameter of type 'FastifyPlugin<{}, Http2Server, Http2ServerRequest, Http2ServerResponse>'.ts(2345)

Can you explain how to make this work correctly?

Hmm this is a tough challenge. Let me check this out locally and start investigating.

@thomheymann do you want to try making the changes I recommended?

Would it be possible to just leave the FastifyPlugin instance type as the ServerIntersection by default? It reflects the reality of plugin authors not knowing what exact server type will be used. For those plugins that depend on a specific server or modified request/response objects those should be able to be passed in but most plugins will be generic.

I've had a stab here at this here:
https://www.typescriptlang.org/play/index.html?ssl=113&ssc=2&pln=108&pc=1#

I mocked http/https/http2 namespaces as the playground couldn't find dependencies

FastifyPlugin instance type is now union of all server types (its called intersection but it's a union, worst naming in typescript ever):

interface FastifyPlugin<Options, Server extends ServerIntersection = ServerIntersection> { ( instance: FastifyInst<ServerIntersection>, opts: Options, next: () => void ): void; }

Plugin authors get correct instance type:

// Generic plugin const p4: FastifyPlugin<{}> = (inst, opts, next) => { console.log(inst.serverRaw.type) // "http" | "https" | "http2" | "https2" } // Specific plugin const p3: FastifyPlugin<{}, https.Server> = (inst, opts, next) => { console.log(inst.serverRaw.type) // "https" }

Userland Fastify instance type is correctly inferred based on options

Userland plugin registration works without typing errors

Seems a good idea to me but my TS is far from good.

Yeah this looks good. I'm happy shipping this now and if things change in the future we can of course change it ourselves.

Yeah this is one of those edge cases that is really hard to solve correctly because it wouldn't happen when using TS from the ground up. If I have the opportunity I'll pass this along to the TS team for consideration.

This PR has been updated but will fail until corresponding Fastify PR has been merged in:
fastify/fastify#2183

tsconfig.json

mcollina

lgtm

mcollina · 2020-04-09T15:23:36Z

@Ethan-Arrowood can you take a look?

Ethan-Arrowood

🚀

thomheymann · 2020-04-09T16:34:32Z

Need to get this one in first to fix build - Travis is bogged:
#86

thomheymann · 2020-04-09T20:14:34Z

@mcollina This isn't going to work in Node 6 unless Fastify is transpiled and published (atm we're pointing to next branch)

/home/runner/work/fastify-plugin/fastify-plugin/node_modules/fastify/lib/reply.js:173
    ...this.raw.getHeaders(),
    ^^^

SyntaxError: Unexpected token ...
    at Object.Module._extensions..js (module.js:586:10)
    at Object.Module._extensions..js (module.js:586:10)

mcollina · 2020-04-09T20:18:24Z

This branch should target node 10+, we 'll release it as a major.

thomheymann · 2020-04-09T20:35:30Z

ok this can be merged in once Fastify v3 has been released

outstanding tasks:
update package.json to Fastify v3

mcollina requested a review from Ethan-Arrowood March 23, 2020 09:30

Ethan-Arrowood suggested changes Mar 24, 2020

View reviewed changes

thomheymann force-pushed the next branch 2 times, most recently from c7fc869 to 5902a02 Compare April 9, 2020 14:30

thomheymann requested a review from Ethan-Arrowood April 9, 2020 14:30

mcollina approved these changes Apr 9, 2020

View reviewed changes

thomheymann force-pushed the next branch 2 times, most recently from 5902a02 to 5e3bcf7 Compare April 9, 2020 16:22

Ethan-Arrowood approved these changes Apr 9, 2020

View reviewed changes

Upgrade to Fastify v3

7e9cdaa

thomheymann force-pushed the next branch from 5e3bcf7 to 7e9cdaa Compare April 9, 2020 20:00

Test Node 10+ only

278b348

thomheymann mentioned this pull request Apr 22, 2020

Fastify v3 plugin upgrade list fastify/fastify#2217

Closed

51 tasks

Upgrade to Fastify v3

01ed77d

thomheymann force-pushed the next branch from 0e692fb to 01ed77d Compare April 23, 2020 09:11

Updated dependencies

1a0a136

mcollina merged commit 5668897 into fastify:master Apr 23, 2020

Ethan-Arrowood mentioned this pull request Jun 29, 2020

add Promise version overload for FastifyPlugin fastify/fastify#2350

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade to Fastify v3 #85

Upgrade to Fastify v3 #85

thomheymann commented Mar 23, 2020 •

edited

mcollina commented Mar 23, 2020

mcollina commented Mar 23, 2020

Ethan-Arrowood commented Mar 23, 2020

thomheymann commented Mar 23, 2020

Ethan-Arrowood commented Mar 24, 2020

Ethan-Arrowood left a comment

Ethan-Arrowood Mar 24, 2020

thomheymann Mar 24, 2020

Ethan-Arrowood Mar 24, 2020

thomheymann Mar 24, 2020

Ethan-Arrowood Mar 29, 2020 •

edited

Ethan-Arrowood Apr 2, 2020

thomheymann Apr 2, 2020

mcollina Apr 2, 2020

Ethan-Arrowood Apr 5, 2020

thomheymann Apr 6, 2020

mcollina left a comment

mcollina commented Apr 9, 2020

Ethan-Arrowood left a comment

thomheymann commented Apr 9, 2020

thomheymann commented Apr 9, 2020

mcollina commented Apr 9, 2020

thomheymann commented Apr 9, 2020

Upgrade to Fastify v3 #85

Upgrade to Fastify v3 #85

Conversation

thomheymann commented Mar 23, 2020 • edited

Example plugin and usage with new FastifyPlugin type

Example plugin with HTTP2 Fastify instance

Checklist

mcollina commented Mar 23, 2020

mcollina commented Mar 23, 2020

Ethan-Arrowood commented Mar 23, 2020

thomheymann commented Mar 23, 2020

Ethan-Arrowood commented Mar 24, 2020

Ethan-Arrowood left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ethan-Arrowood Mar 29, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcollina left a comment

Choose a reason for hiding this comment

mcollina commented Apr 9, 2020

Ethan-Arrowood left a comment

Choose a reason for hiding this comment

thomheymann commented Apr 9, 2020

thomheymann commented Apr 9, 2020

mcollina commented Apr 9, 2020

thomheymann commented Apr 9, 2020

thomheymann commented Mar 23, 2020 •

edited

Example plugin and usage with new `FastifyPlugin` type

Ethan-Arrowood Mar 29, 2020 •

edited