evanw / esbuild

An extremely fast bundler for the web
https://esbuild.github.io/
MIT License
38.11k stars 1.15k forks source link

Feature request: Decorators support #104

Closed fannheyward closed 5 months ago

fannheyward commented 4 years ago

error: Decorators are not supported yet

Any plan to support decorators?

evanw commented 5 months ago

I'm still making progress on this. However, progress is slow because decorators are very complicated. Some things I did recently:

evanw commented 5 months ago

Ok version 0.21.0 is out and now supports the JavaScript decorators proposal. Please try it out and let me know what you think.

@justinfagnani Code size definitely isn't optimal but it shouldn't be that bad I think. I made a trade-off that generates accessor methods at run-time instead of compile-time. It would be interesting to see where esbuild's implementation ends up in your size comparison (is it public somewhere perhaps?).

cayter commented 5 months ago

Ok version 0.21.0 is out and now supports the JavaScript decorators proposal. Please try it out and let me know what you think.

@justinfagnani Code size definitely isn't optimal but it shouldn't be that bad I think. I made a trade-off that generates accessor methods at run-time instead of compile-time. It would be interesting to see where esbuild's implementation ends up in your size comparison (is it public somewhere perhaps?).

This is crazy. I thought this would take few more months to ship. Thanks a lot!

Note that I couldn't get the example in the release working, have to do it like this:

function log(target: any, propertyKey: string, descriptor: PropertyDescriptor) {
    const originalMethod = descriptor.value;
    descriptor.value = function (...args: any[]) {
        console.log(`before ${propertyKey}`);
        const result = originalMethod.apply(this, args);
        console.log(`after ${propertyKey}`);
        return result;
    };
}

class Foo {
    @log
    static foo() {
        console.log("in foo");
    }
}

Foo.foo();
evanw commented 5 months ago

Note that I couldn't get the example in the release working

Here's an example of it working: link. That logs this when run:

before foo
in foo
after foo
cayter commented 5 months ago

Note that I couldn't get the example in the release working

Here's an example of it working: link. That logs this when run:

before foo
in foo
after foo

Ah I didn't specify target. Thanks man!

Obiwarn commented 5 months ago

image

The grind was real.

image

Thank you so much @evanw!

I will test this thoroughly with our internal ORM.

evanw commented 5 months ago

What do people think? Does it work with existing code that uses the JavaScript decorators proposal? Any feedback?

alex3683 commented 5 months ago

EDIT: Just found out you released a new version and this seems to have fixed the issue I had. So from me absolute positiv feedback, since it's also really fast!

Previous comment:

I tried it in our product but got an error with MobX and thus had to return to the transformer workaround. I then tried to make a small reproduction by using your online esbuild tranformer and putting it in a jsfiddle, but there it runs without problems. I'll just post the error and what I found out so far where it goes wrong, but I guess it's not easy to debug.

MobX just throws this: Uncaught TypeError: Cannot convert undefined or null to object (but only for us, not in the fiddle). This is in an initializer of an accessor, where this is undefined. I tried to trace the bug and how esbuild transforms the code, but I didn't really grog the flags parameter that goes in. The only thing I understand is, that when its even, this is undefined, and when it's odd, its set correctly. MobX however is called with an even flag in our production code. I tried to find out what drives the decision within esbuild, but it was too hard for me in a few hours and reverted. I can only offer you to have remote look at my code via teams or something in case you are interested. Or make adjustments that may lead to the cause of this. I guess when this is solved, I'd give you a fully positive feedback :-D

This was my reproduction attempt:

Just to clarify: This may sound like a rant, but is not meant as such. I think you are doing a great job and I assume the cause of the problem could be in our code, but it's hard to find for me and maybe solving this could provide valuable feedback.

hrishikeshs commented 5 months ago

Wow! I was literally trying to write a decorator yesterday to add logging to datadog to our common logger utility. Can't wait to try this out đź‘Ť thank you so much!

marvinruder commented 5 months ago

What do people think? Does it work with existing code that uses the JavaScript decorators proposal? Any feedback?

Works great! I was able to migrate my decorators from the legacy Typescript implementation to the TC39 proposal without problems. ~The only issue I encountered was that class field decorators have no access to the class itself, but this is a limitation of the specification and already discussed there.~ – nevermind, this is possible after the fix in 0.21.2 using this (although one needs to return a function from the decorator for this to work, in an arrow function, this is undefined).

justinfagnani commented 5 months ago

@evanw I tried the Lit decorators, but it looks like metadata isn't implemented yet, which is a blocker for us. I'll be able to test again when metadata support is implemented.

evanw commented 5 months ago

Initial support for decorator metadata has been released in version 0.21.3.

b3nten commented 5 months ago

Evan you rock!

justinfagnani commented 5 months ago

Awesome @evanw!

I ran all of Lit's decorator tests after an esbuild compile and they all pass! I'll check those tests in so if you ever want to check changes against them you can do:

git clone https://github.com/lit/lit.git
cd lit
npm ci
npm link esbuild
npm run test:dev -w @lit/reactive-element