-
Notifications
You must be signed in to change notification settings - Fork 653
mirage: Add default_version
and yanked
to crate
serializer
#9840
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
Conversation
0e04bd5
to
c2b40fc
Compare
mirage/serializers/crate.js
Outdated
'default_version', | ||
'yanked', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are you sure that we need these mentioned here? I would've thought that those are only necessary for the fields that come directly from the corresponding factory/model
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I saw we have
crates.io/mirage/route-handlers/summary.js
Lines 25 to 35 in a46fcdc
just_updated: this.serialize(just_updated).crates.map(it => ({ ...it, versions: null })), | |
most_downloaded: this.serialize(most_downloaded).crates.map(it => ({ ...it, versions: null })), | |
new_crates: this.serialize(new_crates).crates.map(it => ({ ...it, versions: null })), | |
most_recently_downloaded: this.serialize(most_recently_downloaded).crates.map(it => ({ | |
...it, | |
versions: null, | |
})), | |
num_crates, | |
num_downloads, | |
popular_categories: this.serialize(popular_categories).categories, | |
popular_keywords: this.serialize(popular_keywords).keywords, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But after your awareness, I think we might also want to change the factory and fixture as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I understand what you're saying 😅
my question is: do the tests work the same as before when these two lines are removed again? or is something breaking then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, my bad, I thought it was necessary to add these, that was quite embarrassing! A quick inspection of the mirage response confirms your absolute right.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have removed the attrs part.
Should we add these fields to mirage/factory or mirage/fixtures?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we add these fields to mirage/factory or mirage/fixtures?
no :)
factory is for regular fields, model is for relationships, fixtures can die for all I care, and the serializer can be (mis)used for "computed" fields 😉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TIL!!
c2b40fc
to
80d8f97
Compare
This PR adds
default_version
andyanked
to thecrate
serializer.Related:
default_version
on the API #9741yanked
field on the API #9817