Get rid of ts-morph

[vimmerru]

Is your feature request related to a problem? Please describe.
I might be wrong, but I have feeling that ts-morph, related cache infrastructure, non-obvious git-related and deployment issues add more headache than solve problems.

Describe the solution you'd like
Isn't typescript provide enough support to collect required metadata using annotations functions? Just re-write @entity, @Property and relations annotations to collect model metadata in some private field of entity class or some global object. Use metadata collected inside of class instead of cache on file system.

It will simplify and speed up entity initialization approach, allow to reduce limitations on entity naming and file structure, be typescript native.

[B4nan]

I am not interested in dropping tsmorph, there is no way to get the type in other way afaik (reflect metadata have a lot of issues, not usable for this).

I think its enough to let users define all types manually, which will skip the whole ts morph process.

[vimmerru]

(reflect metadata have a lot of issues, not usable for this

Could you provide any example of AST-level data that you can't collect with ts decorators?

I think its enough to let users define all types manually, which will skip the whole ts morph process.

Could you provide any details on this?

[B4nan]

I dont remember fully, but one example is ObjectId type. Also I think I was not able to get pretty much anything more than scalar types. Currently I also use ts morph to get the information about property being nullable. Also for collections I need the full definition of type including the generic parameter.

The whole point of this solution is to reduce duplicate information when defining entity properties. When you define everything manually, nothing will be needed and ts morph wont be needed (and therefore not used).

Take a look at deployment section in docs regarding this (in v3 docs).

[vimmerru]

I dont remember fully, but one example is ObjectId type. Also I think I was not able to get pretty much anything more than scalar types.

It can be just solved by require user to provide you reference type explicitly. In my vision it isn't a big problem to add a bit boilerplate to model definition that will allow to generate typescript native model definition.

@Entity
class User {
  @ManyToOne()
  company?: Company
}

will be just

@Entity
class User {
  @ManyToOne({ref: Company, nullable: true})
  company?: Company
}

A bit of boilerplate that any popular ts library that requires type meta information usually asks, but it will allow to avoid deployment complexity. It isn't so easy in practice. My team spend at least one day to solve deployment issues. Avoid non-native solution with reduced maturity. AST-level code analysis and code generation is definitely anti-pattern in typescript world. It adds non-obvious limitations and unexpected behavior for your library users. I was really surprised why i can't put multiple entities to one file, name files as i want or what is the strange temp folder magically appears in my codebase.

Take a look at deployment section in docs regarding this (in v3 docs).

Doesn't look like acceptable solution for me because model definition is separated from the model type.

[B4nan]

As I said, what you propose is already there, you can always fill those fields and you will skip the whole ts-morph thing. We could also disable caching for that case as it is not really needed. I do like to have it this way, you are the first who is complaining about it, as opposed to multiple other people who liked it.

For deployment, as long as you deploy your source code, everything just works, I myself never have any problem with deploying my apps. And again, you have always the option to provide all types manually (I am planning to add a cli command that will help you find all places where its missing).

I can imagine having some extra interfaces that would require you to pass those attributes, but not as the default.

Doesn't look like acceptable solution for me because model definition is separated from the model type.

Not sure what you mean by that. The page basically just suggests to do the very same thing we are talking about here (filling all types manually).

Btw if you do not like caching, you can always disable it. I come from PHP world and having frameworks creating temp/cache directory is pretty standard there. You can also use different adapter for production, e.g. redis. But generally for production cache is not really needed, the purpose of it is to make development builds faster.

[vimmerru]

Not sure what you mean by that. The page basically just suggests to do the very same thing we are talking about here (filling all types manually).

It is mostly my fault. I just skipped reading because of my internal assumptions and v2 experience. I will try to use type/entity property, but i already see some problems: no good runtime errors in case of missed metadata with very strange stack trances , no static checking that required metadata is provided, this approach isn't well documented. Anyway we are ok with the current state,

[B4nan]

As noted above, I am planning to add command that will check it for you. I'd like to also allow automatic propagation of TS types to decorators so one would only have to verify changes made by orm.

For static analysis we could also have another set of decorators that would require those attributes. In the end, you can create them yourself, simply by reexporting orm's decorators with different typings (or you can enhance only the typings via declaration merging).

We could add them to the ORM by different name, maybe with Strict prefix or suffix? Ideally it should be another package so the names could stay the same, but first I would have to convert this to monorepo which will probably come, but not soon.

[B4nan]

Thinking about the weird error messages you can get (just ran into it myself when designing a testcase) - what about adding some ORM configuration toggle that would trigger proper validation for them? Like strictDecorators and then it would trigger validation inside each decorator that would require the type or entity attributes.

[vimmerru]

Like strictDecorators

Looks good to me. I will try to remember my problems related to errors reporting later this week. Just got one week of vacation and trying to don't read emails )

[lookfirst]

You know what would be rad is to use ts-morph output to automatically rewrite my code to insert the missing metadata using something like tsmod. =) https://github.com/WolkSoftware/tsmod

[B4nan]

You know what would be rad is to use ts-morph output to automatically rewrite my code to insert the missing metadata using something like tsmod. =) https://github.com/WolkSoftware/tsmod

Which is something I was also thinking about.

[vimmerru]

In my vision the most user-friendly way will be:

1. Use model annotations to define metadata as a default approach
2. Provide cli commands to verify model with ts-morph and auto-fix entity classes

[vimmerru]

@B4nan There is one additional problem that can be related to ts-morph. After switch to mikro-orm we noticed significant heap jump on start (at least +400mb). For NodeJS apps it is significant value that may reduce deployment options. For example, deployment on micro instances in AWS.

[B4nan]

But again, if it is related to ts-morph, then providing types manually should solve it.

[vimmerru]

providing types manually should solve it.

I just tried on a small test project to provide all types manually, but seems ts-morph is still called as if there is no cache generated i still see significant heap size jump.

[B4nan]

Can you share it? Also please share how you measure it. I can guarantee that if you fill everything, it wont be used (feel free to check the code in TS metadata provider), but the cause might be elsewhere.

[vimmerru]

mikro-orm-ts-morph.zip

I found the issue. One property was without type. I just simple use console.log(process.memoryUsage()); to get heap size. I attached the project for reference.

Here are some thoughts:

1. Providing 'type' for all properties looks really unnecessary boilerplate.
2. The probability to remember to annotate some field is really high as no any errors reported
3. Analysis of 2 entities require about 40mb of heap. On our real project we see more than 500mb used already, but we expect the model will be significantly complicated in a few month that may cause memory usage peaks painful for us.

[B4nan]

Already started working on the CLI command to find (and possibly fill) missing types.

Providing 'type' for all properties looks really unnecessary boilerplate.

It is used for validation, so it is needed everywhere. But this being boilerplate it the very reason why I went for the ts-morph discovery. I don't want users to repeat themselves.

Btw there is already CLI command to generate production cache, which could be used too to mitigate this issue in production too.

[vimmerru]

@B4nan Can we just allow to explicitly disable ts-morph and fail early if some metadata are missed?

[B4nan]

Yeah we can add config toggle for that as well. Currently this is forced when you set env variable WEBPACK to true, as webpack build also requires it. But it will also force you to use entities array instead of entitiesDirs (which is btw also a way to support multiple entities in one file and overcome the limitations of entity file names).

[B4nan]

Providing 'type' for all properties looks really unnecessary boilerplate.

Just realised you are right, for scalar types I can actually use reflect-metadata so the type could be required only where you do not explicitly state it (e.g. when you define default value - prop = new Prop() instead of prop: Prop = new Prop()), at least for scalar types (and arrays of scalar types). There is still issue with native object types (like Date that is reported as Object), but it will give us some usable defaults so it can be less verbose.

I was under impression that it does not help with user defined classes but looks like it actually works as long as you explicitly state the type. But that is something I can easily tell after the initial discovery.

I will incorporate that as part of the CLI command PR (including also the option to force validation for missing types).

[MichalLytek]

@B4nan
Is there any particular reason why the ts-morph based reflection is not implemented as Typescript compiler plugin, but a runtime parsing of source files with fragile caching?

[B4nan]

Yes, very simple reason, I have zero experience with that :] Also from what I read, you can't do this with tsc, you need to fallback to something like this, right? https://github.com/cevek/ttypescript

So it would not work out of box as with ts-moprh. Also I can imagine this breaking builds with webpack. Also I do not parse anything, the information comes from the TS compiler.

Why do you call the caching fragile? Cache is invalidated based on the actual file contents, it works without problems for me. I do not recall single issue about caching (only one that suggested using relative paths so the cache can be reused in production).

I would be open for additional way to do this, but it does not sound like a proper replacement.

Also note that in v4 ts-morph discovery lives in separate package: #475

[MichalLytek]

Also from what I read, you can't do this with tsc, you need to fallback to something like this, right? cevek/ttypescript
So it would not work out of box as with ts-moprh

ttypescript is well supported by all bundlers or ts-node or nest build.
All you need to do is just declare the plugin in tsconfig.json, then run ts-node -C ttypescript index.ts instead of ts-node index.ts.

Also note that in v4 ts-morph discovery lives in separate package: #475

So just wrap the logic inside the transformer plugin, so users could decide - go with the compiler metadata way or standard runtime parsing.

Why do you call the caching fragile?

How does it work with Docker images? If it doesn't create the metadata on tsc step, I need to run the container (the code) to parse in runtime the source files, and then write the metadata in cache (container instance filesystem). Also for bundlers like webpack we need to distribute the sourcefiles along the bundle.js just to support runtime parsing of the metadata?

I understand that you like this pattern and it works for some people.
But having a clean, well supported and error-prone way to extend the typescript reflection system using ts compiler plugin would be really nice to have.
As you saw, not everybody like this and prefer to stick with fragile reflect-metadata way, just to omit the runtime parsing behavior.

[B4nan]

ttypescript is well supported by all bundlers or ts-node or nest build.

Well supported but you need to adjust all the configs and commands (or even replace them with 3rd party ones, e.g. tsc -> ttsc, personally I would be probably scared of doing this, if I did not know what exactly it does).

So just wrap the logic inside the transformer plugin, so users could decide - go with the compiler metadata way or standard runtime parsing.

As I said, I have no problems with having new optional way to do this. But as I have zero experience with that and I already have a lot of things I am planning to work on for v4 in following months, its something I would rather see as a PR from someone else (who worked with this tool before) :]

How does it work with Docker images? If it doesn't create the metadata on tsc step, I need to run the container (the code) to parse in runtime the source files, and then write the metadata in cache (container instance filesystem).

You can warm up the cache via CLI when building the container if the question is about production deployment (which effectively runs the discovery). Personally I am not a fan of developing apps inside docker containers, there is just too much performance overhead (I guess it is fine on linux, but I am a macOS guy).

Also for bundlers like webpack we need to distribute the sourcefiles along the bundle.js just to support runtime parsing of the metadata?

ts-morph discovery is not compatible with webpack currently, I believe @thekevinbrown is planning to work on that, maybe the solution he had in mind is the same as this, not sure.

[lookfirst]

Developing in docker containers is 🤦