Skip to content
This repository has been archived by the owner on Jun 29, 2021. It is now read-only.

[Poll] #356 Implementation #373

Closed
hasezoey opened this issue Aug 5, 2019 · 12 comments
Closed

[Poll] #356 Implementation #373

hasezoey opened this issue Aug 5, 2019 · 12 comments

Comments

@hasezoey
Copy link
Contributor

hasezoey commented Aug 5, 2019

so feature/4 will be integrated in 6.0.0, but now i wanted to ask what would be better:

  • making the functions static and keep the Class as is, or
    -> this would mean that it is still included in the class which could be problematic in .create({} as Class)
  • outsourcing all the functions and "remove" the class
    -> this would mean that it couldnt be called from the class, but wouldnt have the problem above
  • (or, make both, but one is returning the other (static function returns the outsourced function))

progress on this feature can be seen on https://github.com/hasezoey/typegoose/tree/r6/outsource

(i cant really decide this on my own)
-> for now i will outsource it

@hasezoey
Copy link
Contributor Author

hasezoey commented Aug 5, 2019

i will ping some people i know who work with typegoose, understand it a bit, and were active (in the last ~30 days)
(if you are one of the persons pinged, you dont need to answer this, but it would be good :) )
@Ben305
@mfulop
@PabloSzx
@nodkz
@naorzr

PS: accidentally clicked on "close issue"

@hasezoey hasezoey closed this as completed Aug 5, 2019
@hasezoey hasezoey reopened this Aug 5, 2019
This was referenced Aug 5, 2019
@nodkz
Copy link
Contributor

nodkz commented Aug 5, 2019

Too many changes in master...hasezoey:r6/outsource and it difficult to follow them for making the correct decision. Also, I try to find what exactly feature/4 introduces and without luck.

@naorzr
Copy link

naorzr commented Aug 5, 2019

If it means that we wont have to extend Typegoose and then do new Foo().getModelForClass, I'm all up for that.
Multiple inheritance isn't supported, and mixin is a pain in the ass.
Removing Typegoose class will free up that slot.

@hasezoey
Copy link
Contributor Author

hasezoey commented Aug 5, 2019

@nodkz, @naorzr described it correctly

@nodkz
Copy link
Contributor

nodkz commented Aug 5, 2019

Removing new Foo().getModelForClass sounds good! Need to test it in the real project when a beta will be ready.

But if it will be possible to provide both syntaxes in v6: a new without Typegoose class and the old with Typegoose class - it will be great! It provides a more seamless migration for existing apps.

@hasezoey
Copy link
Contributor Author

hasezoey commented Aug 5, 2019

But if it will be possible to provide both syntaxes in v6: a new without Typegoose class and the old with Typegoose class - it will be great! It provides a more seamless migration for existing apps.

sorry, but this cant be done without breaking the new one...

@hasezoey
Copy link
Contributor Author

hasezoey commented Aug 5, 2019

it is now implement in 72a2da7,
free for anyone to test, but expect changes

and i dont know fully what these lines do, but they work
https://github.com/hasezoey/typegoose/blob/72a2da7b11be1509227820e1285975d6bc8ec338/src/typegoose.ts#L50-L53

@PabloSzx
Copy link
Contributor

PabloSzx commented Aug 6, 2019

Outsourcing the functions seems to be the best idea, since it also improves TypeScript class types for client usage, right now if you want to use the class for types in client TypeScript code, you get getModelForClass and setModelForClass in addition of the actual attributes 😢

@mfulop
Copy link

mfulop commented Aug 7, 2019

and i dont know fully what these lines do, but they work

does this help? :) https://artsy.github.io/blog/2018/11/21/conditional-types-in-typescript/

so it means T if S is assignable to class T (extends a class with their constructor “returning” T) otherwise S

@hasezoey
Copy link
Contributor Author

hasezoey commented Aug 7, 2019

so it means T if S is assignable to class T (extends a class with their constructor “returning” T) otherwise S

i later commits to r6/master, i understood it a bit more, but its still a bit of magic to it...

@hasezoey
Copy link
Contributor Author

hasezoey commented Aug 10, 2019

i will close this because the implementation will stay how it currently is in 6.0.0

currently released under @hasezoey/typegoose@next


when a beta will be ready.

@nodkz it could be considered that 6.0.0 is now in "beta"

@hasezoey
Copy link
Contributor Author

But if it will be possible to provide both syntaxes in v6: a new without Typegoose class and the old with Typegoose class - it will be great! It provides a more seamless migration for existing apps.

@nodkz in version 6.0.0-22 the functions are re-added with a deprecation warning (and just as a pass-through to the new syntax)

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

No branches or pull requests

5 participants