Skip to content
This repository has been archived by the owner on Jan 12, 2023. It is now read-only.

Add types declaration file (resolve #46) #51

Closed
wants to merge 6 commits into from

Conversation

geopic
Copy link

@geopic geopic commented Jan 24, 2020

If you have any queries, let me know.

@geopic geopic requested a review from JaneJeon January 24, 2020 18:51
Copy link
Owner

@JaneJeon JaneJeon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@geopic thanks for the contribution. What you added definitely looks great and checks out, but the issue I stated in JaneJeon/objection-authorize#1 (comment) still persists, which is that the QueryBuilder type is completely erased upon calling Model.query()...

I know I'm asking for a lot, but would you like to try tackling it? I would really appreciate the help, and I would add this in myself but when I tried pasting in the library author's solution, I had a little bit of a problem with the ambiguity of this...

Thank you again!

@geopic
Copy link
Author

geopic commented Jan 28, 2020

No worries @JaneJeon , I'll tackle it today.

@geopic geopic requested a review from JaneJeon January 28, 2020 11:48
index.d.ts Outdated Show resolved Hide resolved
index.d.ts Outdated Show resolved Hide resolved
@geopic geopic requested a review from JaneJeon January 30, 2020 11:31
Copy link
Owner

@JaneJeon JaneJeon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@geopic Hi, how are you verifying these types locally? I've tried everything but I just can't get the types to actually register when I try:

const { Model } = require("objection-2");
const plugin = require(".");

class BaseModel extends plugin(Model) {}
BaseModel.query().insert() // <-- this doesn't work

Have you tried actually calling the plugin and using the returned model with the checkJs tsconfig option turned on (assuming you have typescript installed)?

Copy link
Owner

@JaneJeon JaneJeon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

index.d.ts Outdated Show resolved Hide resolved
index.d.ts Outdated Show resolved Hide resolved
index.d.ts Outdated Show resolved Hide resolved
index.d.ts Outdated Show resolved Hide resolved
@JaneJeon
Copy link
Owner

@geopic on an unrelated note, has this been a learning experience for you? I just wanna make sure you’re actually getting something out of this, too.

@geopic
Copy link
Author

geopic commented Feb 3, 2020

Thanks for the feedback and tips @JaneJeon . I admit I haven't tried chaining anything in my test .ts file but will give it a shot now. The way Objection plugins have to be written to accommodate Objection and its interesting structure is quite new to me. I can write type declaration files but working with an object that returns a modified instance of itself is quite difficult for me to explicitly define in type files at the moment. However, I am learning. 👍 Fingers crossed you will have a working type declaration file. Another thank you for your patience

@JaneJeon
Copy link
Owner

JaneJeon commented Feb 4, 2020

@geopic since it has been a rather lengthy process, I present to you a cheat sheet that you can reference when you're stuck: https://github.com/Vincit/objection.js/blob/master/doc/recipes/plugins.md

Here's some more resources that could help you:

@JaneJeon JaneJeon linked an issue Feb 5, 2020 that may be closed by this pull request
@olavim
Copy link

olavim commented Feb 6, 2020

I noticed my issue being referenced here so I felt obliged to throw in my two cents.

After trying to tackle this problem for a relatively long time, I'm pretty sure perfect typings aren't possible to be created for Objection plugins at the moment. You can get partial support, but once you use two query builder -modifying plugins, each defining a method that modifies the result type, things break down.

I remember pinpointing the issue to the type returned by the custom query building method. Consider methods custom1() and custom2(), each coming from a different plugin:

User.query().custom1().custom2()...

The first issue that arises is that when you call custom1(), by default you return some type such as this. But in plugin 1's query builder, this doesn't contain the method custom2(), and so you get an error trying to do the above query chain. This can be worked around with some magic though. The real issue is that if custom1 and custom2 both change the result type (the results come wrapper in some object, for example), there is no way to solve this, because in plugin 1 you have no way of knowing how plugin 2 changes the result type.

This is a pretty small problem group though, and as far as you're fine with partial typing support, then my code here will probably help.

@geopic
Copy link
Author

geopic commented Feb 10, 2020

Thanks for dropping by and letting us know @olavim.

@geopic
Copy link
Author

geopic commented Feb 18, 2020

So I've tried every approach, every way under the sun to get this working. But it is above my skill level at the minute. I'll push a commit fixing the minor details like hashedFields() being Array<string> etc, and then maybe I or someone else can pick it up in the future when objection.js has a more sensible plugin system. I hope this is ok @JaneJeon.

@olavim
Copy link

olavim commented Feb 19, 2020

I got the existing test cases to work: https://github.com/olavim/objection-hashid

But yeah, Objection's plugin type support is just not there. I have no doubt my code will break immediately after trying some other cases. I had to disable the objection-visibility plugin in my tests since it just resulted in everything being any type (making this whole endeavor moot if even one plugin lacks types).

Moreover, I was unable to change the result type based on the hashIdField static property.

@geopic
Copy link
Author

geopic commented Feb 22, 2020

Brilliant @olavim! You got it to work as best as it probably could 😛

@geopic geopic requested a review from JaneJeon February 22, 2020 21:53
@JaneJeon
Copy link
Owner

@geopic @olavim thank you for your contribution, I'll merge it as soon as I can review the changes from the both of you! (it's been busy at work...)

@geopic
Copy link
Author

geopic commented Mar 1, 2020

@JaneJeon I've pushed another commit based off @olavim's approach. Types work when the module is imported. TS developers will need to use import instead of require. If there are any other details I've missed then let me know, I'd like to get this finished.

@JaneJeon JaneJeon added the wontfix This will not be worked on label Oct 4, 2020
@JaneJeon
Copy link
Owner

JaneJeon commented Oct 4, 2020

RIP as per #61, but I will leave the issue alive.

@JaneJeon JaneJeon closed this Oct 4, 2020
@JaneJeon JaneJeon mentioned this pull request Oct 4, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request wontfix This will not be worked on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TypeScript type declaration file
3 participants