-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Fix and document MixinBuilder
#673
Comments
+1 for fixing it. I would also like to see this moved from I'd also like to see if we can possibly change the behaviour of MixinBuilder to remove the MixinBuilder class entirely and have something like This would simplify the normal way of Mixins (remove the requirement of adding tedious brackets and makes it easier to use imo). If we agree on this approach, we should update our code / tests / docs to use this approach and make this our recommended approach for Mixins. |
+1 for everything @virkt25 said |
I was never a fan of MixinBuilder. I am happy with both proposals: fix it or remove it. 👍 |
+1 to @virkt25's proposal as well. I'm okay to remove it until we fix it :-). |
I have been tried different strategies in PR #1305 to "Fix MixinBuilder to return the correct type for class", but there is no easy way to do it. @dhmlau @shimks Since the mixin builder is a sugar function, users can apply mixins in the chain signature, shall we drop this story from DP3 and revisit it when have more bandwidth? |
Hmm, just took a quick look of the task #1171, if we can define and apply the mixin in the class signature instead of the function, that could potentially solve both two issues....Let me give it a last try! |
I agree. It was not the original intention to spend too much effort for the sugar functions anyway. |
@dhmlau There seems no easy way to get the class signature work either...shall we defer to work on it after DP3? We can get back to it whenever we are confident that there is a feasible solution to apply multiple mixins with a sugar function. |
If we need more research, then I am proposing to defer the work after DP3. Personally, I still think we should remove MixinBuilder completely from our code base and docs. If we agree on this direction, then I think it makes sense to make that happen in DP3, probably as a |
I'm ok with removing |
Seems like we have some consensus.. removing this from |
On a second thought, shall we close this task (because we're not fixing it) and create a new one to remove |
Problem:
The problem of
MixinBuilder
from my test is: functionwith()
doesn't set its returned type as a class extends theBaseClass
:click to see test example
fails when compiling, because compiler doesn't know
myApp
is an instance ofApplication
, therefore doesn't know it has a prototype functionfind()
returning anArray
. The code callsmap
withany
instead ofArray
and fails.This looks like a stop for people wanting to use it...
I am going to defer writing doc for
MixinBuilder
and create an issue for it. I still love the fluent api and would like to dig more ⭐Document cut from PR loopbackio/loopback.io#491
Acceptance Criteria
Move
MixinBuilder
class from@loopback/repository
to@loopback/core
Fix
MixinBuilder
to return the correct type for class as it currently fails to do soUpdate TSDocs for this Class
Tests
Bonus: See if this can be just a static method on
Application
instead of a new Class as proposed here: Fix and documentMixinBuilder
#673 (comment) DO NOT SPEND MORE THAN 1 HOUR on this. Fixing the class is good enough for this task.The text was updated successfully, but these errors were encountered: