WarnerHooh / babel-plugin-parameter-decorator

Function parameter decorator transform plugin for babel v7, just like typescript.
34 stars 9 forks source link

Support parameter decorator in class constructor with public or private keyword #1

Closed jezzgoodwin closed 5 years ago

jezzgoodwin commented 5 years ago

Hello,

Firstly, thank you very much for this babel plugin. It's what we have been looking for since starting to use Babel 7 as our typescript transpiler.

For our ideal use case, we would like to use a parameter decorator in combination with the private or public keyword in a class constructor.

Eg:

constructor(@inject("myService") private myService: MyService) {}

Is this on your roadmap at all?

Many thanks

vjpr commented 5 years ago

I am looking for this too.

@Service()
export class AuthService {
    constructor(
        @Logger(__filename) private log: LoggerInterface,
        @OrmRepository() private userRepository: UserRepository
    ) {
    }
}

Here is the error you get:

TypeError: Property name expected type of string but got null
    at validate (/Users/Vaughan/dev-live/babel-plugin-parameter-decorator/node_modules/.registry.npmjs.org/@babel/types/7.3.4/node_modules/@babel/types/lib/definitions/utils.js:161:13)
    at Object.validate (/Users/Vaughan/dev-live/babel-plugin-parameter-decorator/node_modules/.registry.npmjs.org/@babel/types/7.3.4/node_modules/@babel/types/lib/definitions/utils.js:172:7)
    at validate (/Users/Vaughan/dev-live/babel-plugin-parameter-decorator/node_modules/.registry.npmjs.org/@babel/types/7.3.4/node_modules/@babel/types/lib/validators/validate.js:17:9)
    at builder (/Users/Vaughan/dev-live/babel-plugin-parameter-decorator/node_modules/.registry.npmjs.org/@babel/types/7.3.4/node_modules/@babel/types/lib/builders/builder.js:46:27)
    at Object.Identifier (/Users/Vaughan/dev-live/babel-plugin-parameter-decorator/node_modules/.registry.npmjs.org/@babel/types/7.3.4/node_modules/@babel/types/lib/builders/generated/index.js:321:31)
    at /Users/Vaughan/dev-live/babel-plugin-parameter-decorator/lib/index.js:31:37
    at Array.forEach (<anonymous>)
    at PluginPass.Function (/Users/Vaughan/dev-live/babel-plugin-parameter-decorator/lib/index.js:14:44)
    at newFn (/Users/Vaughan/dev-live/babel-plugin-parameter-decorator/node_modules/.registry.npmjs.org/@babel/traverse/7.3.4/node_modules/@babel/traverse/lib/visitors.js:193:21)
    at NodePath._call (/Users/Vaughan/dev-live/babel-plugin-parameter-decorator/node_modules/.registry.npmjs.org/@babel/traverse/7.3.4/node_modules/@babel/traverse/lib/path/context.js:53:20)
vjpr commented 5 years ago

So the issue is that when you add private, there is not longer param.node.name. It becomes param.node.parameter.name.

Single line fix:

const name = param.node.name || (param.node.parameter && param.node.parameter.name);

@WarnerHooh Could you make this change and publish a new version?

jezzgoodwin commented 5 years ago

I just gave this a try. It seems to only work if the decorator function is in the same file. If the decorator is imported from another file it doesn't seem to work. I'm using WebPack.

vjpr commented 5 years ago

Hmm...it shouldn't really make a difference where the decorator is.

What error do you get?

You are using preset-typescript and ".ts" file ?

jezzgoodwin commented 5 years ago

The error happens at runtime.

The build process works without erroring, and does write the call to the decorator in the javascript. But the function isn't pointing to the WebPack included namespace.

Yes it's preset-typescript and ts files.

vjpr commented 5 years ago

I just pushed a PR with tests. It works fine. Must be something else in your config. Try clearing your cache.

Package available as: @vjpr/babel-plugin-parameter-decorator

vjpr commented 5 years ago

Doesn't work when type of param is ObjectPattern:

    @Query(returns => [Pet])
    public pets(@Ctx() { requestId }: Context): Promise<PetModel[]> {
        this.log.info(`{${requestId}} Find all users`)
        return this.petService.find()
    }

Just have to delete the decorators associated with the param, instead of replacing the param with its name, which doesn't work if the param is an ObjectPattern like above.


Published and PR updated.

jezzgoodwin commented 5 years ago

I'm trying a minimal setup but I still can't get it to work for me.

My .babelrc file:

{
    "presets": [
        "@babel/preset-typescript",
        "@babel/react",
    ],
    "plugins": [
        ["@babel/proposal-decorators", { "legacy": true }],
        ["@babel/proposal-class-properties", { "loose": true }],
        "./parameterDecorator.js" // modified plugin with your fix
    ]
}

My constructor line looks like this:

export class EditStore {
    constructor(@inject(PageStore) private pageStore: PageStore) {
    }
}

Both inject and PageStore are imports from another file.

After building, I am looking at the generated bundle.js file. It contains the following line:

inject(PageStore)(EditStore.prototype, "", 0);

Which doesn't work because in this location both inject and PageStore aren't accessible.

If I remove the private keyword from my constructor and rebuild, I get the following in my build output:

Object(_includes__WEBPACK_IMPORTED_MODULE_1__["inject"])(_PageStore__WEBPACK_IMPORTED_MODULE_2__["PageStore"])(EditStore.prototype, "", 0);

Which does now work because the imports are included properly. But obviously I've lost the private keyword and functionality!

@vjpr are you using webpack for bundling?

vjpr commented 5 years ago

I think it might be to do with webpack's tree shaking. Because we remove the decorstors and types from the AST it thinks it doesn't need to require them anymore.

I will have to look how to fix this.


Also, I'm not sure that "" should be the 2nd argument. This might be another bug.

On Wed 13. Mar 2019 at 09:12, jezzgoodwin notifications@github.com wrote:

I'm trying a minimal setup but I still can't get it to work for me.

My .babelrc file:

{ "presets": [ "@babel/preset-typescript", "@babel/react", ], "plugins": [ ["@babel/proposal-decorators", { "legacy": true }], ["@babel/proposal-class-properties", { "loose": true }], "./parameterDecorator.js" // modified plugin with your fix ] }

My constructor line looks like this:

export class EditStore { constructor(@inject(PageStore) private pageStore: PageStore) { } }

Both inject and PageStore are imports from another file.

After building, I am looking at the generated bundle.js file. It contains the following line:

inject(PageStore)(EditStore.prototype, "", 0);

Which doesn't work because in this location both inject and PageStore aren't accessible.

If I remove the private keyword from my constructor and rebuild, I get the following in my build output:

Object(_includesWEBPACK_IMPORTED_MODULE_1__["inject"])(_PageStoreWEBPACK_IMPORTED_MODULE_2__["PageStore"])(EditStore.prototype, "", 0);

Which does now work because the imports are included properly. But obviously I've lost the private keyword and functionality!

@vjpr https://github.com/vjpr are you using webpack for bundling?

— You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub https://github.com/WarnerHooh/babel-plugin-parameter-decorator/issues/1#issuecomment-472321442, or mute the thread https://github.com/notifications/unsubscribe-auth/AARLRWhGZDhHAa_Nh-qzbgcuZexfrSEiks5vWLL1gaJpZM4bhl0S .

vjpr commented 5 years ago

I have now added webpack test to the repo. Reproduced your error. Now to fix...

Uncaught ReferenceError: validate is not defined
jezzgoodwin commented 5 years ago

This could be an issue in WebPack? I'm not sure at what stage WebPack runs...

vjpr commented 5 years ago

So the issue for me was that I had a file.js and file.ts in the same directory. file.js was being used. file.ts was the tsc transpiled version of the file.

When I removed this, it worked.


Not sure how this happened, maybe not having noEmit in the tsconfig.json. Maybe the IDE was emitting it.

Basically you have to make sure that babel+webpack is compiling a .ts file and not a tsc-transpiled .ts file.


Also, just a note, some prior art to this plugin here:

https://github.com/benderTheCrime/babel-plugin-transform-function-parameter-decorators/blob/develop/src/index.js https://github.com/hqjs/babel-plugin-transform-parameter-decorators/blob/master/index.js

vjpr commented 5 years ago

There may also be an issue with constructor param decorators.

This is what tsc produces:

// If we don't test using a `.ts` file we get:
//
// { SyntaxError: /xxx/babel-plugin-parameter-decorator/test/src/index.js: Unexpected reserved word 'private' (37:26)
//
var __decorate = (this && this.__decorate) || function (decorators, target, key, desc) {
    if (typeof Reflect === "object" && typeof Reflect.decorate === "function") return Reflect.decorate(decorators, target, key, desc);
    switch (arguments.length) {
        case 2: return decorators.reduceRight(function(o, d) { return (d && d(o)) || o; }, target);
        case 3: return decorators.reduceRight(function(o, d) { return (d && d(target, key)), void 0; }, void 0);
        case 4: return decorators.reduceRight(function(o, d) { return (d && d(target, key, o)) || o; }, desc);
    }
};
var __param = (this && this.__param) || function (paramIndex, decorator) {
    return function (target, key) { decorator(target, key, paramIndex); }
};
var decorator_1 = require('./decorator');
var Query = function () { return function () { }; };
var Ctx = function () { return function () { }; };
var Context = {};
var PetModel = {};
var Pet = {};
var Greeter = (function () {
    function Greeter(message) {
        this.message = message;
        this.greeting = message;
    }
    Greeter.prototype.greet = function (name) {
        return "Hello " + name + ", " + this.greeting;
    };
    Greeter.prototype.welcome = function (firstName, lastName) {
        return "Welcome " + lastName + "." + firstName;
    };
    Greeter.prototype.pets = function (_a) {
        var requestId = _a.requestId;
        this.log.info("{" + requestId + "} Find all users");
        return this.petService.find();
    };
    Object.defineProperty(Greeter.prototype, "greet",
        __decorate([
            decorator_1.validate,
            __param(0, decorator_1.required('name'))
        ], Greeter.prototype, "greet", Object.getOwnPropertyDescriptor(Greeter.prototype, "greet")));
    Object.defineProperty(Greeter.prototype, "welcome",
        __decorate([
            decorator_1.validate,
            __param(0, decorator_1.required('firstName')),
            __param(1, decorator_1.required('lastName'))
        ], Greeter.prototype, "welcome", Object.getOwnPropertyDescriptor(Greeter.prototype, "welcome")));
    Object.defineProperty(Greeter.prototype, "pets",
        __decorate([
            Query(function (returns) { return [Pet]; }),
            __param(0, Ctx())
        ], Greeter.prototype, "pets", Object.getOwnPropertyDescriptor(Greeter.prototype, "pets")));
    Greeter = __decorate([
        __param(0, decorator_1.required('message'))
    ], Greeter);
    return Greeter;
})();
exports["default"] = Greeter;

Notice the difference:


// METHOD PARAM

    Object.defineProperty(Greeter.prototype, "pets",
        __decorate([
            Query(function (returns) { return [Pet]; }),
            __param(0, Ctx())
        ], Greeter.prototype, "pets", Object.getOwnPropertyDescriptor(Greeter.prototype, "pets")));

// CONSTRUCTOR PARAM

    Greeter = __decorate([
        __param(0, decorator_1.required('message'))
    ], Greeter);
    return Greeter;
jezzgoodwin commented 5 years ago

There are definitely no .js files in my src folder. I have noEmit set to true in my tsconfig.json file. Are you sure it's working for you with webpack?

I wonder if the issue is to do with the @babel/plugin-proposal-decorators plugin. This plugin depends on it for passing the syntax. Maybe it doesn't deal with the private/public keyword properly.

vjpr commented 5 years ago

I actually don't think any of the available plugins work correctly.

When looking at what TS does, for methods and param decorators, it applies them to Clazz.prototype when the class is declared.

For constructor param decorators, it must replace the the class constructor with a decorated version.


This plugin is broken: https://github.com/hqjs/babel-plugin-transform-parameter-decorators/blob/master/index.js because it applies the param decorators inside the method calls. This means method decorators cannot access param decorations (which would prevent the @validate/@required example working).

WarnerHooh commented 5 years ago

Hello,

Sorry for the delay! The problems are:

  1. The AST of ts and js is different, I can make it compatible to resolve the errors.
  2. The import statement disappearing because of the preset-typescript remove the "type import". https://github.com/babel/babel/blob/29cd27b545689abd72ded684c6bbc6b17ac1b7ca/packages/babel-plugin-transform-typescript/src/index.js#L54-L91
jezzgoodwin commented 5 years ago

@WarnerHooh thanks for the response. Is no 2 something you are able to look at?

WarnerHooh commented 5 years ago

@jezzgoodwin Yap, I am looking into it. But no idea yet. May I ask why you need to compile typescript with babel rather than TSC?

jezzgoodwin commented 5 years ago

We've been trying to use some other babel presets in combination with typescript. Specifically @emotion/babel-preset-css-prop . Using the babel typescript preset allows us to also use this preset.

vjpr commented 5 years ago

@WarnerHooh I managed to fix the problems in a PR. It was a one line fix really. Take a look here https://github.com/WarnerHooh/babel-plugin-parameter-decorator/pull/3

However the bigger problem is that the current plugin won't work for constructor params, e.g:

constructor(@required private foo: Foo) { ... }

This is how TSC compiles method and constructor param decorators:

// METHOD PARAM

    Object.defineProperty(Greeter.prototype, "pets",
        __decorate([
            Query(function (returns) { return [Pet]; }),
            __param(0, Ctx())
        ], Greeter.prototype, "pets", Object.getOwnPropertyDescriptor(Greeter.prototype, "pets")));

// CONSTRUCTOR PARAM

    Greeter = __decorate([
        __param(0, decorator_1.required('message'))
    ], Greeter);
    return Greeter;
WarnerHooh commented 5 years ago

Hi @jezzgoodwin

I don't think the plugin could help with the import issue. If you don't mind making the code a bit messy, you can try add the reference of decorator manually in your code, like:

import {required} from './decorator'

export default class Greeter {
  constructor(@required('greeting') private greeting) {
  }
}
// The tricky way
required;

Or we have to write a babel preset to working with this...

vjpr commented 5 years ago

@jezzgoodwin This looks like your import problem -> https://github.com/babel/babel/issues/8634

jezzgoodwin commented 5 years ago

Thanks for the help guys. I have left a comment on the babel issue. Hopefully someone will be able to pick it up.

WarnerHooh commented 5 years ago

Hi @jezzgoodwin

I have made some change to adapt typescript. Would you please have a look at the dev branch if it's what you want?

jezzgoodwin commented 5 years ago

@WarnerHooh i've just tried using the plugin from the dev branch. I get the following error:

ERROR in ./src/jezz/EditStore.ts
Module build failed (from ./node_modules/babel-loader/lib/index.js):
TypeError: Cannot read property 'type' of undefined
    at isInType (C:\Development\red-deer-react\node_modules\@babel\plugin-transform-typescript\lib\index.js:43:23)
    at isImportTypeOnly (C:\Development\red-deer-react\node_modules\@babel\plugin-transform-typescript\lib\index.js:319:12)
    at PluginPass.Program (C:\Development\red-deer-react\node_modules\@babel\plugin-transform-typescript\lib\index.js:98:30)
    at newFn (C:\Development\red-deer-react\node_modules\@babel\traverse\lib\visitors.js:193:21)
    at NodePath._call (C:\Development\red-deer-react\node_modules\@babel\traverse\lib\path\context.js:53:20)
    at NodePath.call (C:\Development\red-deer-react\node_modules\@babel\traverse\lib\path\context.js:40:17)
    at NodePath.visit (C:\Development\red-deer-react\node_modules\@babel\traverse\lib\path\context.js:88:12)
    at TraversalContext.visitQueue (C:\Development\red-deer-react\node_modules\@babel\traverse\lib\context.js:118:16)
    at TraversalContext.visitSingle (C:\Development\red-deer-react\node_modules\@babel\traverse\lib\context.js:90:19)
    at TraversalContext.visit (C:\Development\red-deer-react\node_modules\@babel\traverse\lib\context.js:146:19)
    at Function.traverse.node (C:\Development\red-deer-react\node_modules\@babel\traverse\lib\index.js:94:17)
    at traverse (C:\Development\red-deer-react\node_modules\@babel\traverse\lib\index.js:76:12)
    at transformFile (C:\Development\red-deer-react\node_modules\@babel\core\lib\transformation\index.js:88:29)
    at runSync (C:\Development\red-deer-react\node_modules\@babel\core\lib\transformation\index.js:45:3)
    at runAsync (C:\Development\red-deer-react\node_modules\@babel\core\lib\transformation\index.js:35:14)
    at process.nextTick (C:\Development\red-deer-react\node_modules\@babel\core\lib\transform.js:34:34)
    at processTicksAndRejections (internal/process/next_tick.js:74:9)
 @ ./src/jezz/Edit.tsx 4:0-40 9:36-45
WarnerHooh commented 5 years ago

Hi @jezzgoodwin

Are you able to provide the code snippet?

jezzgoodwin commented 5 years ago

Yep. It's part of a test project, so the code isn't really doing much yet.

import { action, observable } from "mobx";
import { computedInstance, inject, injectable } from "./includes";
import { PageStore } from "./PageStore";

@injectable("transient")
export class EditStore {
    @observable public id: number = -1;
    @observable public text: string = "";

    constructor(@inject(PageStore) private pageStore: PageStore) {}

    @action
    public componentCreated(id: number) {
        console.log("componentCreated", id);
        this.id = id;
        return () => {
            console.log("componentDestroyed", id);
        };
    }

    @action
    public textChanged = (event: React.FormEvent<HTMLInputElement>) => {
        this.text = event.currentTarget.value;
    };

    @computedInstance
    public getName() {
        return this.pageStore.items[this.id].name;
    }
}
WarnerHooh commented 5 years ago

@jezzgoodwin I made some mistake, can you try it again? 🙏

jezzgoodwin commented 5 years ago

@WarnerHooh there are no longer any errors. But unfortunately it doesn't fix the missing imports.

It is still writing

inject(PageStore)(EditStore.prototype, "", 0);

instead of the webpack version

Object(_includes__WEBPACK_IMPORTED_MODULE_1__["inject"])(_PageStore__WEBPACK_IMPORTED_MODULE_2__["PageStore"])(EditStore.prototype, "", 0);
jezzgoodwin commented 5 years ago

@WarnerHooh do you think it's even possible to fix this issue in your plugin? Maybe the references have already been stripped out by the typescript preset?

WarnerHooh commented 5 years ago

@jezzgoodwin yes, this is really the typescript preset issue. I just wanna have a try. By the way, I get the code compiled like this, which should be correct.

var _includes = require("./includes");
...
(0, _includes.inject)(PageStore)(EditStore, undefined, 0);
jezzgoodwin commented 5 years ago

@WarnerHooh weird. i don't get that. are you using webpack?

WarnerHooh commented 5 years ago

@jezzgoodwin not on webpack, just babel cli. I will have a try with webpack later.

WarnerHooh commented 5 years ago

This should be resolved in the new version 1.0.7.