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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Declaration Merging over scopes in the documentation #3529

Closed
2 tasks done
RafaelGSS opened this issue Dec 13, 2021 · 7 comments
Closed
2 tasks done

Declaration Merging over scopes in the documentation #3529

RafaelGSS opened this issue Dec 13, 2021 · 7 comments
Labels
documentation Improvements or additions to documentation typescript TypeScript related

Comments

@RafaelGSS
Copy link
Member

RafaelGSS commented Dec 13, 2021

Prerequisites

  • I have written a descriptive issue title
  • I have searched existing issues to ensure the feature has not already been requested

馃殌 Feature Proposal

In the documentation, we have some sections approaching the declaration merging, however, looks reasonable to add a declaration merging over scopes as well.

For instance, you have scope X and scope Y:

function scopeX(fastify, opts, done) {
  fastify.register(require('fastify-jwt'), { ...jwtOptions })
  done()
}

function scopeY(fastify, opts, done) {
  // fastify.jwt is undefined, but it shows as a property in the intelisense because of declaration merging
  fastify.jwt
}

fastify.register(scopeX)
fastify.register(scopeY)

Motivation

Fastify was designed to work gracefully with encapsulation, and it should be replicated by their types.

@RafaelGSS RafaelGSS added documentation Improvements or additions to documentation typescript TypeScript related labels Dec 13, 2021
@RafaelGSS
Copy link
Member Author

@fastify/typescript do you know if it's possible to use declaration-merging over scopes by just a configuration in the tsconfig? Looks like a barrier.

@simoneb
Copy link
Contributor

simoneb commented Dec 13, 2021

It would be interesting to understand how declaration merging actually works here.

Please correct me if I'm wrong but I believe that the fastify.jwt decorator is shown in autocompletion because it's exported by the fastify-jwt package, meaning that as soon as you require that package in your code typescript will assume that any occurrence of FastifyInstance in your code will have that decorator.

If that is true, then the problem becomes: how do we restrict declaration merging in a way that it only applies to certain parts of the codebase? It sounds like a tricky problem to solve!

@jsumners
Copy link
Member

If anyone tackles this before #3474 is merged, please branch from it and target that branch in the PR.

@stale
Copy link

stale bot commented Apr 16, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale Issue or pr with more than 15 days of inactivity. label Apr 16, 2022
@RafaelGSS
Copy link
Member Author

I gave it a shot 5 months ago, however, I haven't found a good way to do that. @fastify/typescript @sinclairzx81 do you have any thoughts on that? Otherwise, I'll close it.

@stale stale bot removed the stale Issue or pr with more than 15 days of inactivity. label Apr 16, 2022
@sinclairzx81
Copy link
Contributor

@RafaelGSS Hi

Unfortunately, I don't see a easy way to enable this without changing the signature of register() and forcing either method chaining (as below to allow the context to propagate) or providing generic hints on the scopeX(..) and scopeY(..) functions (as per reference example for this issue). Something like the following.

// remap to Fastify<DefaaultFastifyContext & { jwt: {...} }>
fastify.register((fastify, opts) => { return { jwt: require('fastify-jwt') } })
       .register(async (fastify, opts) => { console.log(fastify.jwt); return {} }) 

One thought might be to drop the done() function in favor of returning a PartialContext | Promise<PartialContext> only where the PartialContext can be Object.assign(fastify, partialContext)'ed and be type remapped to Fastify & PartialContext.

In terms of the scopeX() and scopeY() generic hints, there might be something that can be done with Instantiation expressions (available in TS 4.7) to allow the return type of scopeX() to be inferred as a generic hint on scopeY(), but the provision for this to work would be that the signature for register() is updated accordingly to return an inferable PartialContext

It might be best to defer this for a later time (I think there could be a larger discussion to be had around aligning some of the JS logic to be a bit more conducive towards TS inference) and narrowing some of the ways users can interact with Fastify, this with some notes to other issues I've seen such as #3810 (comment).

Hope that helps!

@RafaelGSS
Copy link
Member Author

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation typescript TypeScript related
Projects
None yet
Development

No branches or pull requests

4 participants