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

typescript doc bug change "import fastify from 'fastify'" to "import { fastify } from 'fastify'" #4241

Closed
2 tasks done
mortifia opened this issue Aug 31, 2022 · 10 comments · Fixed by Eldemarkki/alpomoney-server#14

Comments

@mortifia
Copy link
Contributor

Prerequisites

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

Fastify version

4.5.3

Plugin version

x.x.x

Node.js version

16.16.0

Operating system

Windows

Operating system version (i.e. 20.04, 11.3, 10)

win 11 ( insider )

Description

ts(2349) with current importl in doc when i call "fastify()"

Steps to Reproduce

npm init -y
npm i fastify
npm i -D typescript @types/node
{
  "compilerOptions": {
    ..., //ect
    "target": "ESNext",
    "esModuleInterop": true,
    ..., //ect
  }
}
import fastify from 'fastify'

const server = fastify()

server.get('/ping', async (request, reply) => {
  return 'pong\n'
})

server.listen({ port: 8080 }, (err, address) => {
  if (err) {
    console.error(err)
    process.exit(1)
  }
  console.log(`Server listening at ${address}`)
})

Expected Behavior

no ts(2349) when following example

@mcollina
Copy link
Member

cc @fastify/typescript

@Uzlopak
Copy link
Contributor

Uzlopak commented Sep 1, 2022

Typescript itself says, that you should then set moduleResolution to "node" or "nodeNext". Setting it to "node" or "nodeNext" fixes that issue.

Also see
https://stackoverflow.com/a/57383664/3619994

@Uzlopak Uzlopak closed this as completed Sep 1, 2022
@mortifia
Copy link
Contributor Author

mortifia commented Sep 1, 2022

should add it in the doc of fastify on typescript

by following the documentation as it is, there will be this problem

@Uzlopak
Copy link
Contributor

Uzlopak commented Sep 1, 2022

Imho this is a common typescript issue. Also on the documentation of tsconfig

https://www.typescriptlang.org/tsconfig#target

The special ESNext value refers to the highest version your version of TypeScript supports. This setting should be used with caution, since it doesn’t mean the same thing between different TypeScript versions and can make upgrades less predictable.

So we would be forced to keep our documentation up to date with the typescript versions and tbh I dont think we are responsible to set up the tsconfig properly. It is the responsibility of the programmer to set tsconfig correct and to know what he/she is doing by setting this or that option in tsconfig other than what we recommend in our docs.

You are free to provide a PR to extend the documentation. But I think it is out of scope.

I reopen it to get feedback by @mcollina , @jsumners or others.

@Uzlopak Uzlopak reopened this Sep 1, 2022
@mortifia
Copy link
Contributor Author

mortifia commented Sep 1, 2022

I said that it will be necessary to specify it due to the note on the target

after yes I am talking about it because as it is there will be this error

@Uzlopak
Copy link
Contributor

Uzlopak commented Sep 1, 2022

The strange thing is, that I dont get ts 2349 but ts 2792 if moduleResolution is set to "classic". If I use typescript 3.9.2 I get ts 2307. Using typescript version 4.0.2 up to next (4.9.0-dev.20220901) throws 2792.

And 2792 clearly states to change the moduleResolution to "node".

Other than that, I think an average developer will be able to fix that typescript issue without additional documentation.

But still, why do you get 2349 and not 2792.
Which version of typescript do you use?
Can you please provide a mvce to check why you dont get 2792?

@mortifia
Copy link
Contributor Author

mortifia commented Sep 1, 2022

ps: Setting it to "node" or "nodeNext" in tsconfig dont fix error

node version : v16.16.0

package.json (currently migrate of express to fastify)

{
  "name": "xxxxxx",
  "version": "0.0.0",
  "description": "",
  "type": "module",
  "main": "build/index.js",
  "scripts": {
    "test": "echo \"Error: no test specified\" && exit 1",
    "build": "tsc",
    "start": "node build/index.js",
    "dev": "concurrently \"tsc -w\" \"nodemon build/index.js\""
  },
  "author": "",
  "license": "",
  "dependencies": {
    "@fastify/cors": "^8.1.0",
    "@fastify/swagger": "^7.4.1",
    "@prisma/client": "^3.15.2",
    "bcrypt": "^5.0.1",
    "body-parser": "^1.19.0",
    "cors": "^2.8.5",
    "express": "^4.18.1",
    "fastify": "^4.5.3",
    "jsonwebtoken": "^8.5.1",
    "minio": "^7.0.32",
    "multer": "^1.4.5-lts.1"
  },
  "devDependencies": {
    "@types/bcrypt": "^5.0.0",
    "@types/cors": "^2.8.12",
    "@types/express": "^4.17.13",
    "@types/jsonwebtoken": "^8.5.9",
    "@types/minio": "^7.0.13",
    "@types/multer": "^1.4.7",
    "@types/node": "^18.7.14",
    "concurrently": "^7.3.0",
    "json-schema-to-ts": "^2.5.5",
    "nodemon": "^2.0.19",
    "prisma": "^3.15.2",
    "typescript": "^4.8.2"
  }
}

tsconfig

{
  "compilerOptions": {
    /* Language and Environment */
    "target": "ESNext" /* Set the JavaScript language version for emitted JavaScript and include compatible library declarations. */,

    /* Modules */
    "module": "ESNext" /* Specify what module code is generated. */,
    "rootDir": "./src/" /* Specify the root folder within your source files. */,
    "moduleResolution": "NodeNext" /* Specify how TypeScript looks up a file from a given module specifier. */,

    /* JavaScript Support */

    /* Emit */
    "sourceMap": true /* Create source map files for emitted JavaScript files. */,
    "outDir": "./build" /* Specify an output folder for all emitted files. */,
    /* Interop Constraints */
    "esModuleInterop": true /* Emit additional JavaScript to ease support for importing CommonJS modules. This enables 'allowSyntheticDefaultImports' for type compatibility. */,
    "forceConsistentCasingInFileNames": true /* Ensure that casing is correct in imports. */

    /* Type Checking */
    "strict": true /* Enable all strict type-checking options. */,

    /* Completeness */
    "skipLibCheck": true /* Skip type checking all .d.ts files. */
  },
  "compileOnSave": true,
  "include": ["src/**/*.ts"]
}

src/index.ts (go to minimal to generate erorr)

import fastify from 'fastify'
const server = fastify()

ts generate error on "fastify()" ts(2349)

@climba03003
Copy link
Member

climba03003 commented Sep 1, 2022

It is expected to have error when you use NodeNext with module.
The current typings do not allow this combination.
See: fastify/fastify-cookie#184 and fastify/fastify-static#311

It require heavy refactor to support it.
Even if we change fastify, all the plugins you use need the same refactor to support the specified combination.

@mortifia
Copy link
Contributor Author

mortifia commented Sep 1, 2022

thanks to your respond @climba03003 with : "moduleResolution": "Node" it's ok

ps : should still specify it in the note on the doc ^^

@Uzlopak
Copy link
Contributor

Uzlopak commented Sep 1, 2022

Can you provide a PR?

mortifia added a commit to mortifia/fastify that referenced this issue Sep 1, 2022
add note in typescript doc to prevent ts(2349) warning 

fastify#4241
melroy89 added a commit to melroy89/fastify that referenced this issue Mar 15, 2024
fastify#4241 seems to closed.

Signed-off-by: Melroy van den Berg <melroy@melroy.org>
mcollina pushed a commit that referenced this issue Mar 19, 2024
#4241 seems to closed.

Signed-off-by: Melroy van den Berg <melroy@melroy.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants