vitejs / vite-plugin-react-swc

Speed up your Vite dev server with SWC
MIT License
781 stars 50 forks source link

feat: typescript decorator metadata option, fixes #63 #64

Closed LIMPIX31 closed 1 year ago

LIMPIX31 commented 1 year ago

TypeScript decorator metadata support

Example usage:

export default defineConfig({
  plugins: [react({ 
    tsDecorators: true,
    decoratorMetadata: true
  })]
})
ArnaudBarre commented 1 year ago

The issue is that this not supported by esbuild, so it would require running also SWC at build time like plugins. What is your usecase for decorator metadata?

LIMPIX31 commented 1 year ago

The issue is that this not supported by esbuild, so it would require running also SWC at build time like plugins. What is your usecase for decorator metadata?

I use metadata mainly for dependency injection.

https://github.com/vitejs/vite-plugin-react-swc/blob/d43f7d08184d16d5d61af9c6d03815d1b4d8d593/src/index.ts#L120-L146

Why not just use SWC always?

LIMPIX31 commented 1 year ago

I think it would be best to introduce the enforceSwc property instead of plugins: []. If the user specifies plugins without this property it will throw an error.

Or exclude esbuild from the plugin completely, because the name of the plugin itself is confusing.

ArnaudBarre commented 1 year ago

The idea of the plugin is to use SWC mainly for development speed and keep the default pipeline of Vite that uses esbuild. esbuild will be used anyway to handle minification. And this is such a fast bundler that I think this good for most people to keep compatibility with it (and the spec) for future speed up of tooling more based on it (or alternative like bun).

In an ideal world, esbuild would support fast refresh (I plan to work on it) and we could have a small plugin on top of that and this plugin would always use SWC for people needing things not supported by esbuild (css in js, decorator metadata, ...)

For now you can workaround by adding decoratorMetadata: true directly in the compiled source and using plugins: [] in the plugin config.

What library are you using? (So I can add a test when implementing support for this)

LIMPIX31 commented 1 year ago

The idea of the plugin is to use SWC mainly for development speed and keep the default pipeline of Vite that uses esbuild. esbuild will be used anyway to handle minification. And this is such a fast bundler that I think this good for most people to keep compatibility with it (and the spec) for future speed up of tooling more based on it (or alternative like bun).

In an ideal world, esbuild would support fast refresh (I plan to work on it) and we could have a small plugin on top of that and this plugin would always use SWC for people needing things not supported by esbuild (css in js, decorator metadata, ...)

For now you can workaround by adding decoratorMetadata: true directly in the compiled source and using plugins: [] in the plugin config.

What library are you using? (So I can add a test when implementing support for this)

I know there are a lot of polyfills for reflect. reflect-metadata is the most used.

P.S.It is too bad that esbuild does not support metadata reflection.

LIMPIX31 commented 1 year ago

@ArnaudBarre I can create a PR for the enforceSwc property instead of using plugins: [] since it is not explicit. I also think you should forbid plugins without the enforceSwc property.

Another important addition would be the ability to fully customize the SWC.

ArnaudBarre commented 1 year ago

The issue is not the code, it's making decision on the API surface. I don't want to expose all the configuration of SWC to be able to enforce good defaults. If I add another property that depends on SWC at build time, I will make a breaking change in the config and wrap both in a property like swcConfiguration.

jiby-aurum commented 1 year ago

In an ideal world, esbuild would support fast refresh (I plan to work on it) and we could have a small plugin on top of that and this plugin would always use SWC for people needing things not supported by esbuild (css in js, decorator metadata, ...)

This would be awesome, I currently use unplugin-swc to enable decorator metadata, which unfortunately does not work with this. Having this plugin always using swc and with ability to configure all things swc would be very cool.

jiby-aurum commented 1 year ago

For now you can workaround by adding decoratorMetadata: true directly in the compiled source and using plugins: [] in the plugin config.

this still runs esbuild after swc, would something break if I configure esbuild to false ?

ArnaudBarre commented 1 year ago

this still runs esbuild after swc, would something break if I configure esbuild to false ?

Yeah disabling esbuild breaks the minification step of Vite build. This need some investigation to see if can disable esbuild only for transform but not minification (Using SWC as a minifier could be something interesting in the future, but probably outside of this plugin as this not something React specific).

In my testing running both swc and esbuild is not an issue in perf (<5% increase in build time) that's why it was not prioritise.

jiby-aurum commented 1 year ago

@ArnaudBarre agree, did not notice any issues having both running at all, so I left it there, I am using now with the patch below, and it works well.

diff --git a/index.cjs b/index.cjs
index 15de3934a3a8c76f1b9226fd80f33b97727de5c4..d45a83634c7b01db933029c757fb99d010b28382 100644
--- a/index.cjs
+++ b/index.cjs
@@ -126,12 +126,15 @@ var transformWithOptions = async (id, code, options, reactConfig) => {
       configFile: false,
       sourceMaps: true,
       jsc: {
-        target: "es2020",
+        target: "es2022",
+        keepClassNames: true,
         parser,
         experimental: { plugins: options.plugins },
         transform: {
           useDefineForClassFields: true,
-          react: reactConfig
+          react: reactConfig,
+          legacyDecorator: true,
+          decoratorMetadata: true,
         }
       }
     });

Thanks for the work on the awesome plugin, and looking forward to the plugin being able to do this, without needing the patch.

ArnaudBarre commented 1 year ago

What is your need for keepClassNames? It's true that for builds downgrading tagret before minification with user target is not ideal

jiby-aurum commented 1 year ago

@ArnaudBarre mostly for type-graphql, seemingly it relies on the original class name to determine output graphql type name.

jiby-aurum commented 1 year ago

@ArnaudBarre I found a better patch today

diff --git a/index.cjs b/index.cjs
index 15de3934a3a8c76f1b9226fd80f33b97727de5c4..6df09e725fcd3a2ac5f2fba9cc87543d288061c7 100644
--- a/index.cjs
+++ b/index.cjs
@@ -122,8 +122,7 @@ var transformWithOptions = async (id, code, options, reactConfig) => {
   try {
     result = await (0, import_core.transform)(code, {
       filename: id,
-      swcrc: false,
-      configFile: false,
+      rootMode: "upward",
       sourceMaps: true,
       jsc: {
         target: "es2020",

this lets me then, just drop in a swc configuration file at the root of my mono repo. So instead of exposing all configuration, maybe just expose, two of them, configFile and rootMode. With that all swc configuration can be done outside of the context of the plugin.

LIMPIX31 commented 1 year ago

@ArnaudBarre I found a better patch today

diff --git a/index.cjs b/index.cjs
index 15de3934a3a8c76f1b9226fd80f33b97727de5c4..6df09e725fcd3a2ac5f2fba9cc87543d288061c7 100644
--- a/index.cjs
+++ b/index.cjs
@@ -122,8 +122,7 @@ var transformWithOptions = async (id, code, options, reactConfig) => {
   try {
     result = await (0, import_core.transform)(code, {
       filename: id,
-      swcrc: false,
-      configFile: false,
+      rootMode: "upward",
       sourceMaps: true,
       jsc: {
         target: "es2020",

this lets me then, just drop in a swc configuration file at the root of my mono repo. So instead of exposing all configuration, maybe just expose, two of them, configFile and rootMode. With that all swc configuration can be done outside of the context of the plugin.

This may be an option, but I prefer a configuration that can be shared as a JS modules or NPM package.

I haven't configured vite with the usual vite.config.ts for a long time. Instead, I just export the function call that builds the configuration.

jiby-aurum commented 1 year ago

@LIMPIX31 Sure I see your preference This is what unplugin-swc does, it accepts all swc configuration, but also lets you configure by .swcrc.

Enabling .swcrc does not mean you can still not let the plugin accept swc configuration. I just proposed it as I felt @ArnaudBarre did not want to increase the API Surface.

ArnaudBarre commented 1 year ago

Hi,

TS 5 announced that the --experimentalDecorators and --emitDecoratorMetadata are now deprecated because not compatible with the current spec.

Like for useDefineForClass:

Yep this change in the spec is really annoying, but we think this is not a good idea to make it too simple for people to keep using on this old spec. The new tools in few years will probably not support it and you will not be able to ship them directly to the browser when this is implemented.

If you really want to use the plugin before updating your code, a one line patch is enough.

keidarcy commented 1 year ago

@ArnaudBarre agree, did not notice any issues having both running at all, so I left it there, I am using now with the patch below, and it works well.

diff --git a/index.cjs b/index.cjs
index 15de3934a3a8c76f1b9226fd80f33b97727de5c4..d45a83634c7b01db933029c757fb99d010b28382 100644
--- a/index.cjs
+++ b/index.cjs
@@ -126,12 +126,15 @@ var transformWithOptions = async (id, code, options, reactConfig) => {
       configFile: false,
       sourceMaps: true,
       jsc: {
-        target: "es2020",
+        target: "es2022",
+        keepClassNames: true,
         parser,
         experimental: { plugins: options.plugins },
         transform: {
           useDefineForClassFields: true,
-          react: reactConfig
+          react: reactConfig,
+          legacyDecorator: true,
+          decoratorMetadata: true,
         }
       }
     });

Thanks for the work on the awesome plugin, and looking forward to the plugin being able to do this, without needing the patch.

@jiby-aurum Hi, seems this patch will work but when using reflect-metadata still getting the error below. Do you have any idea?

Reflect.js:545 Uncaught TypeError
    at DecorateConstructor (Reflect.js:545:31)
    at Reflect.decorate (Reflect.js:130:24)
    at _ts_decorate (Decorator.tsx:3:92)
    at DecoratorComponentText.tsx:13:14