Skip to content

Conversation

@TakNePoidet
Copy link
Contributor

Checklist

In practice it turned out that it is better when there are no definitions for locals at all. If necessary, it is easier to determine without conflicts directly in the code

declare module 'fastify' {
  interface FastifyReply {
    locals: Partial<{ assetsManifest: AssetsManifest }> | undefined;
  }
}
server.addHook('preHandler', (request: FastifyRequest, reply: FastifyReply, done: HookHandlerDoneFunction) => {
  if (typeof reply.locals === 'undefined') {
    reply.locals = {};
  }
  reply.locals.assetsManifest = assetsManifest();
    done();
});

@mcollina mcollina requested a review from zekth June 16, 2021 17:13
Copy link
Member

@zekth zekth left a comment

Choose a reason for hiding this comment

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

Indeed that's a good remark. Would you mind write a little Typescript usage in the Readme? Explaining the use of this plugin with Typescript like you did in the test?

README.md Outdated
```typescript
interface Locals {
appVersion: string,
assetsManifest: AssetsManifest,
Copy link
Member

Choose a reason for hiding this comment

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

Why this addition?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved to a separate interface for better readability

This option seems less clear:

declare module 'fastify' {
  interface FastifyReply {
    locals:
      | Partial<{
        appVersion: string;
        assetsManifest: AssetsManifest;
      }>
      | undefined;
    }
}

Copy link
Member

@zekth zekth Jun 17, 2021

Choose a reason for hiding this comment

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

but AssetsManifest is not defined in the example it could lead to misreading don't you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed undeclared types and added another example with an object

interface Locals {
  appVersion: string,
  isAuthorized: boolean,
  user?: {
    id: number
    login: string
  }
}

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

Copy link
Member

@zekth zekth 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!

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.

4 participants