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

Allow refType to be defined in @prop (issue #146) #149

Closed
wants to merge 3 commits into from
Closed

Allow refType to be defined in @prop (issue #146) #149

wants to merge 3 commits into from

Conversation

mattjennings
Copy link

Improving on #148, I restricted the PropOptions.refType to be a union type of strings 'number', 'string', 'Buffer', and 'ObjectID'.

I previously was doing a union type of

mongoose.Schema.Types.Number | 
mongoose.Schema.Types.String | 
mongoose.Schema.Types.ObjectID | 
mongoose.Schema.Types.Buffer

however when mongoose.Schema.Types.Buffer is part of the union type then Typescript seemed to allow any of the mongoose.Schema.Types

Ref has also been modified a bit to cast the correct refType.

A prop example of how it can be used:

  @prop({ ref: Car, refType: 'number' })
  car?: Ref<Car, number>;

restrict PropOptions.refType to number, string, Buffer and ObjectID
@coveralls
Copy link

Coverage Status

Coverage decreased (-2.04%) to 84.971% when pulling 172b8f4 on mattjennings:prop-ref-types into 7f9f876 on szokodiakos:master.

@coveralls
Copy link

coveralls commented May 12, 2018

Coverage Status

Coverage decreased (-2.08%) to 84.928% when pulling 29f0cd7 on mattjennings:prop-ref-types into 7f9f876 on szokodiakos:master.

@mattjennings
Copy link
Author

As an additional note, I was really hoping to figure out a way to just define the refType by doing

  @prop({ ref: Car })
  car?: Ref<Car, number>;

but I'm not familiar enough with reflect-metadata to know what the process might be. I ran into microsoft/TypeScript#9916 which suggests this might not be possible.

@nihiluis
Copy link

any news?

@hasezoey
Copy link
Contributor

hasezoey commented Jul 25, 2019

@mattjennings, when you still work with typegoose / willing todo so, would you resolve the merge conflicts, and try to get news on this?


could someone say to me why someone would do Ref<Car, number> (2 things, not just Ref<Car>)? i dont understand it currently...

EDIT: i think i now know what it means Ref<Class, Type>

@mattjennings
Copy link
Author

Sorry, I haven't used typegoose in over a year (since making the PR), so I'm afraid I'm not familiar enough anymore to address the conflicts.

@hasezoey I think you're right that it meant Ref<Class, Type>, but again, it's been so long I can't really remember.

@hasezoey hasezoey mentioned this pull request Jul 31, 2019
14 tasks
@mfulop
Copy link

mfulop commented Aug 2, 2019

Hi!

Merged, solved conflicts and added tests here (also added itemsRefType):

https://github.com/mfulop/typegoose

Should I post a separate pull request from that repo?

@hasezoey
Copy link
Contributor

hasezoey commented Aug 2, 2019

@mfulop yes, please open a new pr if you have a diffrent repo than here
-> i looked at your repo, please rebase onto master, then i will review your pr

@mfulop
Copy link

mfulop commented Aug 2, 2019

Sure, will do. AFK for a hour or two

@mfulop
Copy link

mfulop commented Aug 2, 2019

I think I did :) please see #369

@hasezoey
Copy link
Contributor

hasezoey commented Aug 2, 2019

@mfulop you dont need to mention it, i am subscribed to the whole project :)

@mfulop
Copy link

mfulop commented Aug 2, 2019

I know I am just not sure the rebase went well

@hasezoey
Copy link
Contributor

hasezoey commented Aug 3, 2019

this can be closed in favor of #369, because it includes tests and is on a newer master

@Ben305

@Ben305 Ben305 closed this Aug 3, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants