QwikDev / qwik

Instant-loading web apps, without effort
https://qwik.dev
MIT License
20.81k stars 1.3k forks source link

[🐞] Qwik City app breaks in dev mode with legacy decorators #5708

Open octet-stream opened 9 months ago

octet-stream commented 9 months ago

Which component is affected?

Qwik Rollup / Vite plugin

Describe the bug

I've been playing around with Qwik and Qwik City building simple demos. Pretty great framework so far, by the way! But when I decided to try it with Mikro ORM I stumbled upon following issue: Qwik breaks with TypeScript legacy decorators and Mikro ORM relies heavily on decorators for database schema definition, so I need those to work properly to be able to use Mikro ORM with Qwik app. The issue appears only in in dev mode.

Reproduction

https://github.com/octet-stream/qwik-legacy-decorators-issue

Steps to reproduce

  1. Clone this repo
  2. Install dependencies via pnpm i
  3. Run pnpm start and you'll see following error
[vite] Pre-transform error: Expression expected
[vite] Pre-transform error: Unexpected token `@`. Expected identifier, string literal, numeric literal or [ for the computed key
[vite] Pre-transform error: Unexpected token `@`. Expected identifier, string literal, numeric literal or [ for the computed key (x2)

This error does not appear in preview mode. Run pnpm preview to verify it. Vite will open your browser and you'll see a todo list app. You will be able to add and remove todos.

System Info

System:
    OS: macOS 14.2.1
    CPU: (10) arm64 Apple M1 Pro
    Memory: 95.73 MB / 16.00 GB
    Shell: 5.9 - /bin/zsh
  Binaries:
    Node: 21.3.0 - ~/Library/Caches/fnm_multishells/9803_1704038600457/bin/node
    Yarn: 1.22.19 - /opt/homebrew/bin/yarn
    npm: 10.2.4 - ~/Library/Caches/fnm_multishells/9803_1704038600457/bin/npm
    pnpm: 8.13.1 - /opt/homebrew/bin/pnpm
    bun: 1.0.21 - /opt/homebrew/bin/bun
  Browsers:
    Chrome: 120.0.6099.216
    Safari: 17.2.1
  npmPackages:
    @builder.io/qwik: ^1.3.5 => 1.3.5
    @builder.io/qwik-city: ^1.3.5 => 1.3.5
    undici: * => 6.3.0
    vite: ^5.0.11 => 5.0.11

Additional Information

I saw similar issue, but the error is still there and happens only in dev mode, so I believe there's a way to make it work for dev mode as well without bringing some additional plugins. Also, from what I can tell reading the comments the solution with babel plugin doesn't work.

octet-stream commented 9 months ago

I found a workaround with vite-plugin-typescript-transform

This is how to use it:

In vite.config.ts add this plugin to plugins list (can be placed anywhere) and set enforce option to pre, then override jsx option to preserve, so that it will be transformed later

import ts from "typescript"

import {defineConfig, type UserConfig} from "vite"
import {qwikVite} from "@builder.io/qwik/optimizer"
import {qwikCity} from "@builder.io/qwik-city/vite"
import tsconfigPaths from "vite-tsconfig-paths"

import {vitePluginTypescriptTransform} from "vite-plugin-typescript-transform"

export default defineConfig(() => ({
   plugins: [
     vitePluginTypescriptTransform({
      enforce: "pre",
      filter: {
        files: {
          include: /.tsx?$/,
        }
      },
      tsconfig: {
        override: {
          jsx: ts.JsxEmit.Preserve
        }
      }
    }),
    qwikCity({trailingSlash: false}),
    qwikVite(),
    tsconfigPaths()
  ]
}))

This is not the solution and I believe this have to be supported out of the box, because decorators is common pattern.

Also, as far as I can tell, the problem occurs because rollup can't parse modules with decorators, so this is a compilation problem. And as I said before - it happens only in dev mode and I have no idea why.

wmertens commented 9 months ago

In production, the optimizer will transform the code, and it looks like it supports decorators. In dev mode, it's either not involved or only slightly involved, and rollup will have to handle the decorators.

So the solution would be either to run the optimizer in dev mode, which might be hard especially with hot reload (no idea), or to add decorator support to rollup somehow.

Since not everyone is using decorators, I think this should be a user side configuration, except if it can be supported with minimal overhead, in which case a PR is welcome.

JerryWu1234 commented 8 months ago

@octet-stream

import { defineConfig, UserConfig } from 'vite'
import { qwikVite } from '@builder.io/qwik/optimizer'
import { qwikCity } from '@builder.io/qwik-city/vite'
import tsconfigPaths from 'vite-tsconfig-paths'
import { babel } from '@rollup/plugin-babel'

export default defineConfig((): UserConfig => ({
  plugins: [

    qwikCity({ trailingSlash: false }),
    qwikVite(),
    tsconfigPaths(),
    babel({
      babelHelpers: 'bundled',
      extensions: ['.js', '.jsx', '.es6', '.es', '.mjs', '.ts', '.tsx'],
      plugins: [
        ['@babel/plugin-proposal-decorators', { legacy: true }],
        ['@babel/plugin-proposal-class-properties', { loose: true }]
      ],
    }),
  ],
  server: {
    headers: {
      'Cache-Control': 'public, max-age=0'
    }
  },
  preview: {
    headers: {
      'Cache-Control': 'public, max-age=600'
    }
  }
}))

it works fine if utilizing babel. @wmertens what do i do for qwik ? I saw qwik directly run the Vite in the Dev, offer a plugin? or run by qwik, such as qwik dev?

JerryWu1234 commented 8 months ago

image

wmertens commented 8 months ago

@JerryWu1234 I don't understand what you mean.

In any case, you can change the config to only run babel during dev, and maybe only on the files with legacy decorators.

You could also use a codemod to remove the legacy decorators.

wmertens commented 8 months ago

Ok I figured it out, so the optimizer does allow decorator syntax but it doesn't transpile it, not even in prod mode. During dev, vite does not add the vite:esbuild plugin, which does transpile it, so that's when it errors out.

I'm working on the optimizer so I'll see if I can just enable decorator syntax transpiling.

JerryWu1234 commented 7 months ago

Ok I figured it out, so the optimizer does allow decorator syntax but it doesn't transpile it, not even in prod mode. During dev, vite does not add the vite:esbuild plugin, which does transpile it, so that's when it errors out.

I'm working on the optimizer so I'll see if I can just enable decorator syntax transpiling.

I'm not good at Rust, meanwhile I also have checked the result of the optimizer; which didn't any transplie surely, so I'll close this PR,however, please let me know if there is anything else I can do I'll pick other issue up,

wmertens commented 7 months ago

Ok I know where to do it but I don't know how to get swc to do it. I asked in their discord but no response so far.

octet-stream commented 7 months ago

You mean how to get it to transpile decorators?

This works for legacy decorators and emits metadata:

{
  "jsc": {
    "parser": {
      "syntax": "typescript",
      "tsx": false,
      "decorators": true
    },
    "transform": {
      "legacyDecorator": true,
      "decoratorMetadata": true
    }
  }
}

Here's an example:

https://play.swc.rs/?version=1.3.100&code=H4sIAAAAAAAAA2WOMQvCQAyF9%2FsVcZEKLq6HVgWdRHToXo5rLKVt7khT9Cj9754i7eCWvPfyvUjwCDcvlSPTbLMUdpDBEoY8dz9xr2EzKvXoyX4EuLPzyBKSFQwKgFF6JpjsAq1jI44TMVyiaOipJvekNdQY9HR%2BwRABo4pofHnHArYxXQfXcCapJHzhh7ksbmRaXOj52064ojJVf0HLaASLo8T0KU6x5A3KR8IC6AAAAA%3D%3D&config=H4sIAAAAAAAAA32RO27DMAyG95zC4JyhHVoEnbvmDAUhU4ZavUAyQYzAd6%2FsWK5bG10kkd%2FPp%2B6HpoFPMfDW3MuzGBlZiBe7eKSPirfiAe0ziWGXFY6VqozIohdafC2ZxKiJpSDlC01geHBQxig2cVgX8dSh6d9r4By3zXgmxRYVdxMjd6RjoyQvc4fgUxL63SEEF53t1%2BVNCplJZDNKwNh5WknHOil7upL%2FKy7oiyh%2FGI8iEQNt0lWF%2FZdmdldUKnfKeyJHpx2voEV2z08VzWQ41HNaE4TUXlYDTX%2F6WNkr%2FIjqgpYi4ORcI6fFD9%2Brl3J4OwIAAA%3D%3D

wmertens commented 7 months ago

I meant using the rust API which is what Qwik is using... I'm getting there

wmertens commented 2 months ago

Everything is in place to enable this now, except figuring out how to do the transform with SWC.

JerryWu1234 commented 1 month ago

Everything is in place to enable this now, except figuring out how to do the transform with SWC.

@wmertens I saw you already knew how to enable it so that DId you submit a PR?