Decorator Execution Order about typegoose

[hasezoey]

I think this fork will became the new "official" repo.

szokodiakos#387 got merged, it is now officially the new official

[hasezoey]

i think its better to be called in buildSchema / _buildSchema, because a class decorator is optional

[fabioformosa]

It's work fine now, very nice work guy!
I owe one beer, let's add a donation button into the typegoose doc page.
Thank you!
PS: A final suggestion... IMHO, Typegoose would raise an exception if someone uses two classes with the same name, without disambiguate them by collectionName or customName.

[hasezoey]

Possible Solution:

• cache all decorator evaulations
• cache only inner-class ones

Cache Solutions:

• cache using Map<T, S>
• i dont know any other

Execution Solutions:

• execute the cached ones in buildSchema / _buildSchema
• i dont know any other

Drawback

• more memory consumption, because of storing functions

@B-Stefan what do you think?

[fabioformosa]

Exactly, I fell in this trap just trying to add a @TypeAlias as class decorator.
Look at this:
https://stackoverflow.com/questions/57707455/typescript-decorator-get-class-decorator-value-from-method-decorator

I disagree with the answer, I think it's better to use a buffer to cache all metadata produced by method decorator, until class decorator is called.

[hasezoey]

encountered a problem: caching is good and all, but how to make differing classes with the same name when schemaOptions nor options can be accessed in @prop? i couldnt find anything that makes a class / function unique ... so for now i made an (undocumented) variable __uniqueID

after doing the change to cached version, 3 tests fail, and i cannot figure out why (because it didnt fail before the cache change, and this only wraps all into another function)

WIP branch: https://github.com/hasezoey/typegoose/tree/r6/cache-inner-class-decorators

[hasezoey]

i looked into it again, didnt got it work, though about another possible solution:
"strip" the logic (that calls schemas and co) and do move them into _buildSchema

Update: found the issue, it is because the subSchemas (that dont have a call to make model, which should not be needed) got not build, and so it was "empty", i currently dont know a fix without forcing the user to always make a model out of it

[hasezoey]

Update: added c8a5a18fc35b9c5461c0d78585ff566d57d550a7, which fixes the "build on subdocuments" error, but new ones show up, i think about for now (version 6) disabling the customName functions

PS: for something like this, it seems more easy to just "reinterpret"(/"recreate") mongoose into typescript

Update: could reduce it to 1 test failing (biguser)

[B-Stefan]

@hasezoey Thanks for working on this!
You are absolutely right - I ran into this issue yesterday again at another combination of nested entities.

[fabioformosa]

I'm going to pull your branch to try to solve the failing test. My proj constraint is to have more classes with the same name, this feature is strictly necessary to me. It would be a pity to give up, you're doing a very impressive work, I think this fork will became the new "official" repo.

[hasezoey]

Update: could theoretically fix "all" the errors, except:

• "_id: false" is just not working on @prop, only on @arrayProp

---

@fabioformosa Please know that i have 4(and more) unpushed commits, and as i said above (in this comment) i almost fixed everything

[fabioformosa]

Ok, great! Let me know when you push the new version 6.0.0-27, so I try it in my project.
It's ok at the moment not use _id: false

[hasezoey]

It's ok at the moment not use _id: false

its a requirement in biguser's test currently, and clutters the database

Note:
currently a cache (that caches the build schemas (buildSchemas in data.ts), but even after re-applying with schema.set("_id", false) it didnt even change)

Update: made Automattic/mongoose#8137, as long as i dont get an update on it, i will disable the test(requirement) for it, and add a note, but as long as it persists, i cant release 6.0.0

[hasezoey]

Update: i made it now commented out, and all tests pass, will squash all the WIP commits, and @fabioformosa could you evaluate/test it? (will make a pr, and update this comment)

Update: its #24

[B-Stefan]

@hasezoey This is working with my branch except for one test
expect(foundUser.job.jobType).to.not.have.property('_id'); is failing

[hasezoey]

@B-Stefan even the new 58490e8?

[B-Stefan]

Did not tested this commit, yet.

[B-Stefan]

With 58490e8 all tests are green! 👍 nice work!

[hasezoey]

nice work!

i would say its "not nice work" more like "hacky work" / "deactivating an existing feature" (@prop({ _id: false }) dosnt work anymore / only in arrayprop sometimes??)

PS: i really hope the mongoose issue will get resolved fast ... and that 5.7 gets finally released, tomorrow i will look into this (_id: false) once more, why it worked before, and now not, will report then again

Report from next day: i investigated it again, came to the conclusion to wait for Automattic/mongoose#8137 to either support "inline _id:false" or fix ?schema.set('_id', false) or delete schema.paths._id works...

[fabioformosa]

I pushed a commit in this sample project https://github.com/fabioformosa/typegoose-validation-error (you know it) branch typegoosev6.
Add to package.json the package of typegoose from branch r6/cache-inner-class-decorators.
One test fails :(

[hasezoey]

easy fix: i made i mistake, it should have been __uniqueID, but when i set it it was _uniqueID (notice the double underscore)

fixed in 9f77b49

PS: @fabioformosa i pushed an updated version to your repo (including that @hasezoey/typegoose is now a git-npm module, if you dont use github & git with ssh, please change to https)

[hasezoey]

@fabioformosa for now, the customName thing is disabled until this is fixed...

[B-Stefan]

PS: A final suggestion... IMHO, Typegoose would raise an exception if someone uses two classes with the same name, without disambiguate them by collectionName or customName.

Actually I ran into this a couple of days ago.
I had two classes with the same name and had to debug the reason why some properties where missing. An Error would be great!

[fabioformosa]

@fabioformosa for now, the customName thing is disabled until this is fixed...

Ah ok. Which is the unsolved issue?

[hasezoey]

Ah ok. Which is the unsolved issue?

#23 (this) | #24 (pr for this)

[hasezoey]

@fabioformosa @B-Stefan is this soft error enough? 33e5014
or should it throw instead of being "soft"?

[fabioformosa]

All tests pass! I tried also here https://github.com/fabioformosa/typegoose-validation-error and it's ok!
The only known issue is that about _id: false in @prop, isn't?

[hasezoey]

yes, that is why it currently dosnt get merged, because _id: false is an existing feature, which worked, and now somehow dosnt ... and until now i didnt got a response in the mongoose issue for it

[hasezoey]

PR got squash Merged in ada5cfb