Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support for Property Accessors #184

Closed
MikeBKemp opened this issue May 21, 2021 · 6 comments
Closed

Support for Property Accessors #184

MikeBKemp opened this issue May 21, 2021 · 6 comments

Comments

@MikeBKemp
Copy link

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
Copy link
Owner

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
Copy link
Author

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 typeorm/typeorm#3548 (comment), 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
Copy link
Author

Hmm, I've just realised that I have another issue. I'm writing a React Native app with the Hermes engine, and facebook/hermes#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
Copy link
Owner

IRCraziestTaxi commented May 25, 2021

@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
Copy link

Huxpro commented Jul 27, 2021

@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 facebook/hermes#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 facebook/hermes#114 (comment)

@MikeBKemp
Copy link
Author

@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 facebook/hermes#114 (comment) certainly looks very promising and should fix my issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants