The function form of @Resolver(of => Foo) doesn't work when using @babel/preset-typescript

[vjpr]

Describe the bug

In getObjectType in the Resolver decorator, to check whether an object type or a type func is passed in, the prototype property is checked. When using babel, anonymous functions are transpiled to named functions, which have prototypes like so:

> (of => Foo).prototype
undefined

> (function(of) { return Foo }).prototype
{constructor: ƒ}

 const getObjectType = objectTypeOrTypeFunc
            ? objectTypeOrTypeFunc.prototype
                ? () => objectTypeOrTypeFunc
                : objectTypeOrTypeFunc
            : () => {
                throw new Error(`No provided object type in '@Resolver' decorator for class '${target.name}!'`);
            };

So the check will wrap the transpiled function into another function, and then an error is thrown in buildFieldResolverMetadata saying cannot find fields on null.

Also, why of => Foo vs Foo - its just sugar right?

To Reproduce

Use preset @babel/typescript and plugin https://github.com/leonardfactory/babel-plugin-transform-typescript-metadata

Create a resolver with @Resolver(of => Foo).

Expected behavior

Should work. But instead it should throw a cannot find fields on null error.

Logs

N/A

Enviorment (please complete the following information):

• OS: [e.g. Windows] macOS
• Node (e.g. 10.5.0) 12
• Package version [e.g. 0.12.2] (please check if the bug still exist in newest release) 0.17.2
• TypeScript version (e.g. 2.8.2) 3.4.5

Additional context
Add any other context about the problem here.

Workaround

Don't use Resolver(of => Foo) when using Babel, use just Resolver(Foo)

[MichalLytek]

@vjpr
You need to have your code transpiled to ES2016, not ES5 that doesn't support arrow functions.
https://typegraphql.ml/docs/installation.html#typescript-configuration

Also, JS or babel/typescript is not officially supported (#55).
With #296 coming in the future, you will miss a lot of goodness by sticking to the alternative compiler, so it might be better to just use TypeScript.

[vjpr]

You need to have your code transpiled to ES2016, not ES5 that doesn't support arrow functions.
typegraphql.ml/docs/installation.html#typescript-configuration

It would be a tiny change to support ES5. Just check for a function instead of object. https://stackoverflow.com/questions/5999998/check-if-a-variable-is-of-function-type

The requirement to use ES2016 can cause a lot of additional complexity for Babel users. There is a good discussion of the problems that occur here: Vincit/objection.js#388. You need to add an overrides config to your Babel config, and if you interact with third-party ES5 libraries, there are issues when overriding classes, etc.

So for simplicity sake, I think the small change would help. And the .prototype check doesn't seem like a perfect solution anyway. Its wierd for anon functions to work, and functions not to.

With #296 coming in the future, you will miss a lot of goodness by sticking to the alternative compiler, so it might be better to just use TypeScript.

We can easily modify this plugin (leonardfactory/babel-plugin-transform-typescript-metadata) to add the missing type information so that it is compatible with Babel too.

[MichalLytek]

We can easily modify this plugin (leonardfactory/babel-plugin-transform-typescript-metadata) to add the missing type information so that it is compatible with Babel too.

Easily? Go ahead, I will provide official support for Babel if you deliver fully working types reflection plugin 😉

Current Pitfalls
We cannot know if type annotations are just types (i.e. IMyInterface) or concrete values (like classes, etc.).

So it's not possible to do with Babel as we need full type info e.g. about enums.

There is a good discussion of the problems that occur here: Vincit/objection.js#388.

What exactly problems do you have with transpiling your code to 4 years standard in Node.js environment?

and if you interact with third-party ES5 libraries, there are issues when overriding classes, etc.

For me it's interoperable:

function Foo() {
  console.log("Foo");
}

Foo.prototype.test = function() {
  console.log("test");
};

const f = new Foo();
f.test();

class Bar extends Foo {
  constructor() {
    super();
    console.log("Bar");
  }
}

const b = new Bar();
b.test();

[vjpr]

Easily? Go ahead, I will provide official support for Babel if you deliver fully working types reflection plugin 😉

Opened an issue: Easily? Go ahead, I will provide official support for Babel if you deliver fully working types reflection plugin 😉

So it's not possible to do with Babel as we need full type info e.g. about enums.

Could you explain this more, I'm not exactly sure about it. In the plugin's docs, it seems like it can get full type info but cannot tell if it's a class or an interface. For an enum, we could create a new object, and add the metadata to it to tell that its an enum...something like that. Haven't looked into it deeply though yet.

[MichalLytek]

Without type checker or language service, you can rely only the : Type syntax. So you can't use type inference, e.g. detecting return type from a method and you can't traverse through type aliases like NullableArray. Also, you can't navigate to the interfaces and use it's property names and types.

The current TypeScript metadata reflection is limited and babel architecture is limiting too. I wan't to create an experience for developers with truly transparent schema generation from normal, not limited TS types.

For now there are more important things to do than official fully-working Babel support. I can think of that only after standardizing the new ES decorators syntax.

So closing this as a duplicate of #55 and #300 🔒

[vjpr]

@19majkel94 So no chance of changing the function check? I think it's a very small change.

[vjpr]

What exactly problems do you have with transpiling your code to 4 years standard in Node.js environment?

If you go to the issue I sent, you will see a detailed discussion. It's not as easy as you think because most of third party Node packages are still ES5. Objection argued against it for a while, then supported it.

[MichalLytek]

@vjpr
https://stackoverflow.com/questions/5999998/check-if-a-variable-is-of-function-type

If you go to the issue I sent, you will see a detailed discussion.

I've read that and haven't spotted any real issue about using ES5 3rd party code in ES6 app codebase.

The npm distributed version of TypeGraphQL is also compiled to es2016 with classes syntax.

[vjpr]

The fact that you must pass an anonymous function instead of a normal function is bizarre. It will catch people out. I think that's enough of a reason to change it.

screenshot

I'm sure there is a way to differentiate functions from classes...

[MichalLytek]

It will catch people out.

14 months, 2000 stars, and you are the first one that complains and doesn't want to change compiler to ES6 to make that work 😉

The fact that you must pass an anonymous function instead of a normal function is bizarre.

It's the only way to distinguish a thunk from class. Changing this API is a breaking change and will piss Nest users off. Maybe in v1.0 with larger refactor and breaking changes I will decide to stick to () => foo syntax everywhere as a convention - right now @Field() have thunk but implements not, etc.

[vjpr]

doesn't want to change compiler to ES6 to make that work

I have a huge monorepo project with hundreds of packages frontend and backend. It's not that easy to just change the compiler. If you look at the issue I sent you will find many reports of this issue. Also google "Class constructor Model cannot be invoked without 'new'". ES5 is much more prevalent in the Node ecosystem. As soon as you only support ES6, you cause problems for people.

Anyway, its clear now you don't want it so I will leave it. I will just have to change all the decorator parameter thunks to classes.

Why do you allow thunks anyway? Is it syntactic sugar or for other reasons?

Maybe in v1.0 with larger refactor and breaking changes I will decide to stick to () => foo syntax everywhere as a convention

Please don't, this would break people who transpile to ES5 without a workaround.

[MichalLytek]

Why do you allow thunks anyway? Is it syntactic sugar or for other reasons?

Thunks are the only way to handle circular references. It was adopted as a convention everywhere in TypeGraphQL, thanks to that I have no issues about circular dependencies.

Please don't, this would break people who transpile to ES5 without a workaround.

Why? Without "thunk or not" problem I don't have to check the .prototype and just assume that it's a thunk, so call it to get a reference to the class.

ES5 is much more prevalent in the Node ecosystem. As soon as you only support ES6, you cause problems for people.

TypeGraphQL works only with ES6 because it heavily uses reflection like Function.name. It doesn't support older versions of Node nor compilation to ES5.

[vjpr]

Why? Without "thunk or not" problem I don't have to check the .prototype and just assume that it's a thunk, so call it to get a reference to the class.

My bad, yes that would solve my problem.