IRCraziestTaxi / typeorm-linq-repository

Wraps TypeORM repository pattern and QueryBuilder using fluent, LINQ-style queries.
MIT License
185 stars 10 forks source link

Support for Property Accessors #184

Closed MikeBKemp closed 3 years ago

MikeBKemp commented 3 years ago

Hi - do you plan to support property accessors? i.e.:

new LinqRepository(Entity).getOne()
    .joinAlso(e => e["someChild"])

I've patched ts-simple-nameof locally to get it to work for me, by changing one of the regular expressions:

diff --git a/node_modules/ts-simple-nameof/src/nameof.js b/node_modules/ts-simple-nameof/src/nameof.js
index e084038..7c3e4d9 100644
--- a/node_modules/ts-simple-nameof/src/nameof.js
+++ b/node_modules/ts-simple-nameof/src/nameof.js
@@ -23,7 +23,7 @@ function nameof(nameFunction, options) {
     //   "function(x){return x.prop}"
     // FYI - during local dev testing i observed carriage returns after the curly brackets as well
     // Note by maintainer: See https://github.com/IRCraziestTaxi/ts-simple-nameof/pull/13#issuecomment-567171802 for explanation of this regex.
-    var matchRegex = /function\s*\(\w+\)\s*\{[\r\n\s]*return\s+\w+\.((\w+\.)*(\w+))/i;
+    var matchRegex = /function\s*\(\w+\)\s*\{[\r\n\s]*return\s+\w+(?:\[\"|\.)((\w+\.)*(\w+))/i;
     var es5Match = fnStr.match(matchRegex);
     if (es5Match) {
         return (options && options.lastProp)

I appreciate such a change may be counter to your goals for the package (or maybe it's just a bad idea!).

IRCraziestTaxi commented 3 years ago

Hey @MikeBKemp, thanks for using typeorm-linq-repository! I always appreciate feedback.

I guess my first question is... why do you want to use property accessors with it? If accessing a property that is not defined on the class, that breaks type safety, which is the entire goal of this project. Same with ts-simple-nameof, which is where the change would need to occur.

I am hesitant to make that change, but before making that call, I'd like to hear your use case for using property accessors instead of .joinAlso(e => e.someChild) so I'm not dismissing a legitimate use case.

MikeBKemp commented 3 years ago

HI @IRCraziestTaxi! Thanks for the response. The use case for me is private fields - I have several entities with foreign keys on private fields (for encapsulation of entity behaviour). I was using QueryBuilder to join on these keys - similar to this comment https://github.com/typeorm/typeorm/issues/3548#issuecomment-472989734, but of course there's no type safety at all there. Using typeorm-linq-repository with property accessors gives me that, as typescript gives an error if the accessor is wrong.

I appreciate that may a niche use case though.

MikeBKemp commented 3 years ago

Hmm, I've just realised that I have another issue. I'm writing a React Native app with the Hermes engine, and https://github.com/facebook/hermes/issues/114 breaks ts-simple-nameof. I may need to use a compile-time solution like https://github.com/dsherret/ts-nameof alongside QueryBuilder instead.

IRCraziestTaxi commented 3 years ago

@MikeBKemp I'm glad you found a solution that works out for you!

For the sake of any future users with the same issue, I would say that if you are joining on a relation, I would consider it a public property anyway. Adding support for property accessors and, in general, breaking type safety would not be a good solution.

Again, thanks for using typeorm-linq-repository and supporting it!

Huxpro commented 3 years ago

@MikeBKemp

I'm writing a React Native app with the Hermes engine, and facebook/hermes#114 breaks ts-simple-nameof.

I don't know anything about ts-simple-nameof but if your use case was blocked by https://github.com/facebook/hermes/issues/114, we are adding a special directive "show source" to make toString returning original source code. Would you mind try it with React Native 0.65-rc.3 and see if it helps? You can find more details at https://github.com/facebook/hermes/issues/114#issuecomment-887106990

MikeBKemp commented 3 years ago

@Huxpro

Thanks for letting me know about the change. Unfortunately I cannot get the RC to build (probably some of my dependencies are incompatible, and it would take some time to recreate my use case in a fresh project). However https://github.com/facebook/hermes/issues/114#issuecomment-887106990 certainly looks very promising and should fix my issue.