fastify / fastify-cli

Run a Fastify application with one command!
MIT License
658 stars 167 forks source link

Configuration for using tsup #487

Open segevfiner opened 2 years ago

segevfiner commented 2 years ago

Prerequisites

🚀 Feature Proposal

In #485 we talked about possibly using tsup instead of tsc, so here is some workable configuration for doing so for when you will want to implement it. This configuration doesn't bundle to a single file as I'm unsure if fastify-autoload will actually work with that.

P.S. I also tried WebPack but it self-destructs on fastify-autoload due to bundling. :man_shrugging:

The code

tsup.config.ts ```ts import { defineConfig } from 'tsup'; export default defineConfig({ entry: ['src/app.ts'], bundle: false, sourcemap: true, dts: true, clean: true, }); ```
package.json scripts ```json "scripts": { "test": "yarn build:ts && tsc -p test/tsconfig.json && tap --ts test/**/*.test.ts", "start": "yarn build:ts --env.NODE_ENV production && fastify start -l info dist/app.js", "build:bin": "yarn build:ts --env.NODE_ENV production && pkg .", "build:ts": "tsup", "watch:ts": "tsup --watch", "dev": "npm run build:ts && concurrently -k -p \"[{name}]\" -n \"TypeScript,App\" -c \"yellow.bold,cyan.bold\" \"npm:watch:ts\" \"npm:dev:start\"", "dev:start": "fastify start --ignore-watch=.ts$ -w -l info -P dist/app.js", }, ```
tasks.json ```json { "version": "2.0.0", "tasks": [ { "type": "npm", "script": "watch:ts", "group": { "kind": "build", "isDefault": true }, "problemMatcher": [ { "owner": "tsup", "source": "tsup", "severity": "error", "pattern":[ { "regexp": "^✘\\s+\\[ERROR\\]\\s+(.+)$", "message": 1 }, { "regexp": "^\\s*$" }, { "regexp": "^\\s+(.+):(\\d+):(\\d+):$", "file": 1, "line": 2, "column": 3 } ], "background": { "beginsPattern": "^CJS Build start$", "endsPattern": "^CJS .* Build success|^CJS Build failed" } }, { "base": "$tsc", "background": { "beginsPattern": "^DTS Build start$", "endsPattern": "^DTS .* Build success|^DTS Build failed" } }, ], "label": "npm: watch:ts", "detail": "tsc -w", "isBackground": true, "presentation": { "reveal": "never", "group": "watchers" } }, ] } ```

Notes

Motivation

tsup is faster, and supports additional features, such as define replacement, bundling (Which I'm not sure will work with fastify-autoload), minification, etc.

This should probably be an option, whether it's the default or tsc depends on experimenting with how well it works.

I can contribute it as a PR, but we do need to decide how to add an option and organize the templates around it first.

Example

No response

mcollina commented 2 years ago

I'm not sure that bundling would work at all, so this might be a non-starter.

segevfiner commented 2 years ago

@mcollina ~It doesn't bundle by default. Separate files just like tsc does, this configuration works for me.~ OK, looks like it does bundle, I updated the config to disable that

climba03003 commented 2 years ago

I don't really understand why the TypeScript template do not follow how JavaScript does. It should not require a separate process to build and then restart.

What it needs is --register ts-node/register.

package.json is more simple and clean. It is basically identical to JavaScript one.

{
  "scripts": {
    "test": "tap --ts test/**/*.test.ts",
    "start": "npm run build:ts && fastify start -l info dist/app.js",
    "build:ts": "tsc",
    "dev": "fastify start -r ts-node/register -w -l info -P src/app.ts"
  }
}
climba03003 commented 2 years ago

The initial one actually added in https://github.com/fastify/fastify-cli/pull/227 I think we should simplify the TypeScript template. It do not need any more tools. cc @fastify/typescript ping for advise.

segevfiner commented 2 years ago

One reason might be that ts-node is kinda finicky, but there are alternatives to it that might work better. Some users also might prefer distributing transpiled code rather than TypeScript and having it transpiled at runtime.

As I said in the previous PR, there are many ways fo transpile TS and it's quite opinionated.

climba03003 commented 2 years ago

One reason might be that ts-node is kinda finicky, but there are alternatives to it that might work better. Some users also might prefer distributing transpiled code rather than TypeScript and having it transpiled at runtime.

As I said in the previous PR, there are many ways fo transpile TS and it's quite opinionated.

We can consider ts-node as official tool and we already include it inside the package.json Why don't we make use of it?

ts-node may not be the most performant, but it should be a good starting point.

segevfiner commented 2 years ago

One problem I can see with it is a tendency to explode on ESM in node_modules, but I'm not sure if vanilla tsc doesn't explode on that either.

But, yeah it is already required and used by tap.

segevfiner commented 2 years ago

P.S. I suggested offering a CLI flag to allow a choice.

segevfiner commented 2 years ago

There is also tsx/esno that I heard Vite is examining.

fox1t commented 2 years ago

@climba03003 I am not a fan of ts-node because it works differently than compiling + executing the compiled code with Node.js It also has different rules for what is compiled (basically ignores everything that is not imported directly by a compiled file). This is also different from how tsc works. Moreover, sometimes stack traces are not working as expected. Finally, the build script is needed, so we use Node.js to run compiled JS in dev, making the build more predictable.

That said, I am not sure about the bundling part, @mcollina. Would you mind elaborating? Here we are replacing tsc with tsup for dev builds. No bundling should be involved at all. The output will be the same as from tsc itself.

edit: ok I get the @mcollina point now, and I agree with him.

climba03003 commented 2 years ago

@climba03003 I am not a fan of ts-node because it works differently than compiling + executing the compiled code with Node.js It also has different rules for what is compiled (basically ignores everything that is not imported directly by a compiled file). This is also different from how tsc works. Moreover, sometimes stack traces are not working as expected. Finally, the build script is needed, so we use Node.js to run compiled JS in dev, making the build more predictable.

Running compiled code in dev meanings you will not get to the direct point of that error. The error stack will show where it throw in the compiled version. You need extra effort to find it yourself.

I don't think fastify-cli will reflect source-map.

$ fastify start -r ts-node/register -w -l info -P src/app.ts
Error
    at app (/<root>/src/app.ts:32:9)
    at Plugin.exec (/<root>/node_modules/avvio/plugin.js:132:19)
    at Boot.loadPlugin (/<root>/node_modules/avvio/plugin.js:274:10)
    at processTicksAndRejections (node:internal/process/task_queues:82:21)
$ npm run build:ts && concurrently -k -p "[{name}]" -n "TypeScript,App" -c "yellow.bold,cyan.bold" "npm:watch:ts" "npm:dev:start"
Error
     at app (/<root>/dist/app.js:15:11)
     at Plugin.exec (/<root>/node_modules/avvio/plugin.js:132:19)    
     at Boot.loadPlugin (/<root>/node_modules/avvio/plugin.js:274:10)
     at process.processTicksAndRejections (node:internal/process/task_queues:82:21)

Moreover, sometimes stack traces are not working as expected.

I think I never face this problem. Can you give me an example?

segevfiner commented 2 years ago

So maybe we need something like https://github.com/evanw/node-source-map-support when working with a configuration that needs source maps for stack traces? Assuming there is nothing built-in nowadays.

climba03003 commented 2 years ago

So maybe we need something like https://github.com/evanw/node-source-map-support when working with a configuration that needs source maps for stack traces? Assuming there is nothing built-in nowadays.

Yes, the current approach need to add more and more thing in order to support a convenient development environment.

I accept that ts-node is not good enough and sometimes it even worse, but comparing both approach. It should be the simple one and good for starter.

For advanced user, they can change to use their own stack in this case.

fox1t commented 2 years ago

Running compiled code in dev meanings you will not get to the direct point of that error. The error stack will show where it throw in the compiled version. You need extra effort to find it yourself.

I don't think fastify-cli will reflect source-map.

$ fastify start -r ts-node/register -w -l info -P src/app.ts
Error
    at app (/<root>/src/app.ts:32:9)
    at Plugin.exec (/<root>/node_modules/avvio/plugin.js:132:19)
    at Boot.loadPlugin (/<root>/node_modules/avvio/plugin.js:274:10)
    at processTicksAndRejections (node:internal/process/task_queues:82:21)
$ npm run build:ts && concurrently -k -p "[{name}]" -n "TypeScript,App" -c "yellow.bold,cyan.bold" "npm:watch:ts" "npm:dev:start"
Error
     at app (/<root>/dist/app.js:15:11)
     at Plugin.exec (/<root>/node_modules/avvio/plugin.js:132:19)    
     at Boot.loadPlugin (/<root>/node_modules/avvio/plugin.js:274:10)
     at process.processTicksAndRejections (node:internal/process/task_queues:82:21)

Moreover, sometimes stack traces are not working as expected.

I think I never face this problem. Can you give me an example?

Actually, Node.js has built-in support for source maps since v12: https://nodejs.org/dist/latest-v12.x/docs/api/cli.html#cli_enable_source_maps It is experimental, but for dev env, it is excellent to use it. I don't think that using tsup raises the complexity bar too much. What I usually use is visible here: https://github.com/fox1t/typescript-microservice-starter/blob/master/package.json#L14 (@cspotcode/source-map-support/register isn't needed anymore).

climba03003 commented 2 years ago

Actually, Node.js has built-in support for source maps since v12: https://nodejs.org/dist/latest-v12.x/docs/api/cli.html#cli_enable_source_maps It is experimental, but for dev env, it is excellent to use it. I don't think that using tsup raises the complexity bar too much. What I usually use is visible here: https://github.com/fox1t/typescript-microservice-starter/blob/master/package.json#L14 (@cspotcode/source-map-support/register isn't needed anymore).

My intention here is keep thing as simple as possible. More tools mean the user need to learn and know it before actually start coding.

In my observation in the Discord server. Most of the people use fastify-cli is beginner in fastify. They should focus more on how to use fastify instead of learning the tools first.

Thanks for raising --enable-source-maps flag. I do not know much about node core flag. And I think most of the node user do not know much about it too. Another question would be, how can we enable this flag when using fastify-cli? export NODE_OPTIONS='--enable-source-maps' && npm run build:ts && concurrently -k -p "[{name}]" -n "TypeScript,App" -c "yellow.bold,cyan.bold" "npm:watch:ts" "npm:dev:start" Would it be too long and not good for beginner?

fox1t commented 2 years ago

I agree with you 100% about the simple as a possible approach. I am not sure why --enable-source-maps is completely forgotten by the community. What about keeping tsc/ts-node as default and letting users opt-in for tsup by passing an additional flag to the cli? I am also fine if we decide to use only tsc/ts-node, but we should be aware that we are limiting the possibilities of the cli. Bigger projects always use tsc alternatives in development to avoid that "JAVA-like" compile times. :)

climba03003 commented 2 years ago

What about keeping tsc/ts-node as default and letting users opt-in for tsup by passing an additional flag to the cli?

A flag or an interactive selection interface would be great.

fox1t commented 2 years ago

I want an interactive selection interface too!!!

segevfiner commented 2 years ago

Might be useful if you do go for it: https://marketplace.visualstudio.com/items?itemName=segevfiner.tsup-problem-matcher, hopefully it works well 😝

meotimdihia commented 1 year ago

@segevfiner Did you do ts-up work with Fastify? I can't do it.

segevfiner commented 1 year ago

@meotimdihia Yeah. In a proprietary project, but I can post a sample if you want, it's basically just fastify generate and replacing the tsc commands with tsup, and adding a type-check command to invoke tsc --noEmit.

meotimdihia commented 1 year ago

@segevfiner Currently, I am using the configuration:

export default defineConfig({
  entry: [
    "src/index.ts",
    "src/plugins/**/*.ts",
    "src/routes/**/*.ts",
    "prisma/**/*.ts"
  ],
  bundle: false,
  platform: "node",
  target: "node18",
  sourcemap: true,
  dts: true,
  clean: true
})

I often get the error:

node:events:491
      throw er; // Unhandled 'error' event
      ^

Error [ERR_WORKER_OUT_OF_MEMORY]: Worker terminated due to reaching memory limit: JS heap out of memory
    at new NodeError (node:internal/errors:400:5)
    at [kOnExit] (node:internal/worker:277:26)
    at Worker.<computed>.onexit (node:internal/worker:199:20)
Emitted 'error' event on Worker instance at:
    at [kOnExit] (node:internal/worker:277:12)
    at Worker.<computed>.onexit (node:internal/worker:199:20) {
  code: 'ERR_WORKER_OUT_OF_MEMORY'
}

Sometimes, it is other bugs...How did you build and keep the directory's structure?

segevfiner commented 1 year ago

I'm not sure if that is from your tsup or fastify process?

But for memory errors, it might be that your app is just big, the --max-old-space-size flag for Node often helps, otherwise if it is from tsup, it is something to be reported, as it might not be handling the large amount of unbundled files in your scenario as well as it could, in my scenario, we used bundling (Meaning I also add to remove @fastify/autoload which doesn't work with bundling).

Other bugs, should be reported similarily to the appropriate project's issue tracker depending on their origin, and assuming they are not caused by app code or some other third party module you happen to depend on. That's assuming you have the time to do so while submitting enough details for someone to reproduce and take a look.

meotimdihia commented 1 year ago

@segevfiner I think we don't need to remove @fastify/autoload with the configuration: tsup.config.ts

import { defineConfig } from "tsup"

export default defineConfig({
  entry: [
    "src/web/index.ts",
    "src/web/plugins/**/*.ts",
    "src/web/routes/**/*.ts",
    "prisma/**/*.ts"
  ],
  bundle: true,
  platform: "node",
  target: "node18",
  sourcemap: true,
  clean: true
})
segevfiner commented 1 year ago

I had trouble with such a config without chunk splitting due to duplicate modules in each plugin bundle.