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

Migration of the infamous triplet to next level, Nodenext support #4349

Closed
2 tasks done
climba03003 opened this issue Oct 17, 2022 · 31 comments
Closed
2 tasks done

Migration of the infamous triplet to next level, Nodenext support #4349

climba03003 opened this issue Oct 17, 2022 · 31 comments
Labels
good first issue Good for newcomers semver-minor Issue or PR that should land as semver minor

Comments

@climba03003
Copy link
Member

climba03003 commented Oct 17, 2022

Prerequisites

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

Issue

Currently, we aim to support different way of import and TypeScript environment which is great for developer experience.
However, the tech is growing and the current architect of the infamous triplet facing some limitation.

  1. Named export in ESM for JavaScript. expose named export fastify-cors#231
  2. TypeScript support including NodeNext and IDE typings. make type definitons "module": "nodenext" compatible fastify-cookie#184

Solution

Here is the summary on a proper approach that should be done inside the organization packages.

  1. Refactor to use explicit module.exports.

The reason behind is that node itself are using static analysis on CommonJS. Which leads to the problem, dynamic generation of named export by fastify-plugin will only be recognize by TypeScript but not Node.

// index.js
const fp = require('fastify-plugin')

function plugin() {}

// const plugin = require('plugin')
// import plugin from 'plugin' - ESM
module.exports = fp(plugin)
// import plugin from 'plugin' - TypeScript
module.exports.default = plugin
// const { plugin } = require('plugin')
// import { plugin } from 'plugin'
module.exports.plugin= plugin
  1. Refactor the typings and use namespace

The current typings is relay on the TypeScript to do module resolution and which will break the NodeNext moduleResolution.

// index.d.ts

import { FastifyPluginCallback } from 'fastify'

type Plugin = FastifyPluginCallback<plugin.PluginOptions>

declare namespace plugin {
  // plugin options
  export interface PluginOptions {}

  // other types export

  // named export
  // import { plugin } from 'plugin'
  // const { plugin } = require('plugin')
  export const plugin: Plugin
  // default export
  // import plugin from 'plugin'
  export { plugin as default }
}

declare function plugin(...params: Parameters<Plugin>): ReturnType<Plugin>

// CJS export
// const plugin = require('plugin')
export = plugin
@mcollina
Copy link
Member

Is the new system backward compatible?

@fox1t you had an old testbed repo for these. Should we update it for the new testcase/problem?

@climba03003
Copy link
Member Author

Is the new system backward compatible?

Yes, it support all the previous use case.

@Uzlopak
Copy link
Contributor

Uzlopak commented Oct 17, 2022

What do we do with fastify-plugin?

@climba03003
Copy link
Member Author

climba03003 commented Oct 17, 2022

What do we do with fastify-plugin?

It's becomes a tools only for providing metadata and exclude encapsulation.

@fox1t
Copy link
Member

fox1t commented Oct 17, 2022

@mcollina yes, this was the playground that allowed us to discover that triplet https://github.com/fox1t/modules-playground
I am updating it with the new information, replacing the part that says to use export default with export = namespace crafted as above. I want to check that all of the other rules still apply.

@fox1t
Copy link
Member

fox1t commented Oct 21, 2022

@mcollina and @climba03003 I've updated the playground https://github.com/fox1t/modules-playground

We have the winner, the "infamous triplet with namespace types".

fastify.fastify = fastify;
fastify.default = fastify;
module.exports = fastify
declare namespace fastify {
  export { fastify };
  export { fastify as default };
}

declare function fastify(): string

export = fastify;

This combination is usable in every possible context, without any issues. It is also the most compatible one (even better than TS compiled to CJS that lacks of namespace callable import O_o)

@mcollina
Copy link
Member

Are we rolling this out across all org?

@climba03003
Copy link
Member Author

Are we rolling this out across all org?

Certainly YES, it also be a good chance for the contribution.

@cesarvspr
Copy link
Contributor

Are we rolling this out across all org?

Certainly YES, it also be a good chance for the contribution.

do we have this issue in the respective repos that require this change?

@climba03003
Copy link
Member Author

do we have this issue in the respective repos that require this change?

It's a hard work. There are too many repo and creating issue one by one use more time than just sending the fix.
I would be glad, if someone would be volunteering. 😆

@cesarvspr
Copy link
Contributor

cesarvspr commented Oct 24, 2022

It's a hard work. There are too many repo and creating issue one by one use more time than just sending the fix. I would be glad, if someone would be volunteering. laughing

I think I will just pick a repo and work on it 😄

Could you say briefly looking at https://github.com/fastify/fastify-multipart/blob/master/index.d.ts and confirm the need for this export update?

@climba03003
Copy link
Member Author

climba03003 commented Oct 24, 2022

Could you say briefly looking at https://github.com/fastify/fastify-multipart/blob/master/index.d.ts and confirm the need for this export update?

Screenshot 2022-10-24 at 18-56-00 fastify-multipart_index d ts at master · fastify_fastify-multipart

Orange - Internal Types
Green - Exported Types
Blue - Declaration Migration

  1. Internal Types remains the same
  2. Exported Types wrap inside a namespace
  3. Declaration Migration move to the top for visibility and organization
  4. add explicit plugin types
  5. add declare function do merge with namespace
  6. export = in the end of file
  7. update entry-point file module.exports

@cesarvspr
Copy link
Contributor

please review fastify/fastify-multipart#396

@cesarvspr
Copy link
Contributor

https://github.com/fastify/fastify-helmet/blob/master/types/index.d.ts also needs the same kind of update, right? @climba03003

@Uzlopak Uzlopak changed the title Migration of the infamous triplet to next level Migration of the infamous triplet to next level, Nodenext support Nov 21, 2022
@Uzlopak
Copy link
Contributor

Uzlopak commented Nov 22, 2022

The minimal approach to make the typings nodenext compatible is I think done with #4438

If we want to improve chaining and other stuff, then we need to invest more time.

@Uzlopak
Copy link
Contributor

Uzlopak commented Nov 23, 2022

@kibertoad

I checked your claim on twitter regarding esModuleInterop. It seems to work without issues.

@fox1t
Copy link
Member

fox1t commented Nov 23, 2022

@Uzlopak, which claim?

@Uzlopak
Copy link
Contributor

Uzlopak commented Nov 23, 2022

That export = plugin could fail when esModuleInterop: false

@fox1t
Copy link
Member

fox1t commented Nov 23, 2022

This is the case you are referring to: https://github.com/fox1t/modules-playground/blob/master/modules/ts-es-module-interop-false/cjs-namespace.ts

As far as I can see there are no fail cases: cjs-namespace is always importable.

@climba03003
Copy link
Member Author

climba03003 commented Nov 23, 2022

Just need to know the TypeScript and Node module system deeper.
The types works because we do something on the module.exports exportation.
When esModuleInterop: false,

import default from 'package'
// import.js
// const default = require('package').default
// export.js
// module.exports.default = package
 
import * as namespace from 'package'
// import.js
//  const namespace = require('package')
// export.js
// module.exports = namespace

import { default as package } from 'package'
// import.js
// const { default: package } = require('package').default
// export.js
// module.exports = package
// module.exports.default = package // we recursively adding .default

@Uzlopak
Copy link
Contributor

Uzlopak commented Nov 23, 2022

So there are now about 10 PRs regarding various plugins. It takes about 10 minutes to convert typings for nodenext. Sometimes it takes more time, because some packages can be optimized in a "drive-by".

I guess we can first process the open PRs and then I can provide another batch.

@Uzlopak
Copy link
Contributor

Uzlopak commented Nov 28, 2022

I think these are repos we have to make nodenext compatible:

  • accept-negotiator
  • ajv-compiler
  • any-schema-you-like
  • avvio
  • aws-lambda-fastify
  • busboy
  • compile-schemas-to-typescript
  • create-fastify
  • csrf
  • csrf-protection
  • deepmerge
  • env-schema
  • fastify
  • fastify-accepts
  • fastify-accepts-serializer
  • fastify-api
  • fastify-auth
  • fastify-autoload
  • fastify-awilix
  • fastify-basic-auth
  • fastify-bearer-auth
  • fastify-caching
  • fastify-circuit-breaker
  • fastify-citgm
  • fastify-cli
  • fastify-compress
  • fastify-cookie
  • fastify-cors
  • fastify-diagnostics-channel
  • fastify-dx
  • fastify-early-hints
  • fastify-elasticsearch
  • fastify-env
  • fastify-error
  • fastify-etag
  • fastify-express
  • fastify-flash
  • fastify-formbody
  • fastify-funky
  • fastify-helmet
  • fastify-hotwire
  • fastify-http-proxy
  • fastify-jwt
  • fastify-kafka
  • fastify-leveldb
  • fastify-mongodb
  • fastify-multipart
  • fastify-mysql
  • fastify-nextjs
  • fastify-oauth2
  • fastify-passport
  • fastify-plugin
  • fastify-postgres
  • fastify-rate-limit
  • fastify-redis
  • fastify-reply-from
  • fastify-request-context
  • fastify-response-validation
  • fastify-routes
  • fastify-routes-stats
  • fastify-schedule
  • fastify-secure-session
  • fastify-sensible
  • fastify-soap-client
  • fastify-static
  • fastify-swagger
  • fastify-swagger-ui
  • fastify-type-provider-fluent-json-schema
  • fastify-type-provider-json-schema-to-ts
  • fastify-type-provider-typebox
  • fastify-typescript-extended-sample
  • fastify-url-data
  • fastify-websocket
  • fastify-zipkin
  • fast-json-stringify
  • fast-json-stringify-compiler
  • fast-proxy
  • fast-uri
  • fluent-json-schema
  • forwarded
  • light-my-request
  • middie
  • one-line-logger
  • point-of-view
  • process-warning
  • proxy-addr
  • restartable
  • safe-regex2
  • secure-json-parse
  • session
  • skeleton
  • under-pressure

@Uzlopak
Copy link
Contributor

Uzlopak commented Dec 9, 2022

We are imho nearly done. The typescript ones i wont touch.

But on the other hand avvio, fluent-json-schema and co are kind of hard to transform.

@Fdawgs
Copy link
Member

Fdawgs commented Jul 2, 2023

We are imho nearly done. The typescript ones i wont touch.

But on the other hand avvio, fluent-json-schema and co are kind of hard to transform.

Just wanted to say good job and thanks for smashing all of these @Uzlopak! I've started using TypeScript to type check JS projects and the types being all up to date for these has been great.

@mertcanaltin
Copy link

still need help? I want to make my first contribution it's a great chance for me @mcollina @fox1t @Uzlopak @Fdawgs @climba03003

@Eomm
Copy link
Member

Eomm commented Aug 26, 2023

I think the work is maily done reading this comment: #4349 (comment)

we should check if the missing ✅ are expected or not

@Uzlopak
Copy link
Contributor

Uzlopak commented Aug 26, 2023

I started to rewrite the typings for avvio few weeks ago, but it is very hard to implement them properly.

@fox1t
Copy link
Member

fox1t commented Aug 28, 2023

@Uzlopak, it is truly a complex task. GL & HF.

@nicobao
Copy link

nicobao commented Nov 30, 2023

Hi there! Thanks for the god-like work! 🙏

I faced an error with Node16, it seems it's not taking my tsconfig.json "paths" into consideration and can't resolve things like @/shared/file.js on runtime after build (while it works in dev mode).
While trying to debug it, I found this issue: fastify/fastify-helmet#208, which is pretty much unrelated except he uses Node16 as well. In there, @climba03003 linked to this very issue about NodeNext, saying Node16 isn't fully supported, and there is this warning on your website:

Note 2: Avoid using "moduleResolution": "NodeNext" in tsconfig.json with "type": "module" in package.json. This combination is currently not supported by fastify typing system. [ts(2349)](https://github.com/fastify/fastify/issues/4241) warning.

which I took into consideration to choose "Node16". However, I now realize that Node16 ~= NodeNext, and fastify is not yet fully compatible with it either.
Except for the "paths" problem, I don't have any issue with Node16 at the moment.
Should I expect some errors?

Thanks again for the awesome work!

@climba03003
Copy link
Member Author

climba03003 commented Mar 18, 2024

I believe all the plugins works are done.
Most of the others is dedicated library or already written in TypeScript.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers semver-minor Issue or PR that should land as semver minor
Projects
None yet
Development

No branches or pull requests

9 participants