feat: @discordjs/structures #8417
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 2 Skipped Deployments
|
github-actions
bot
requested review from
iCrawl,
kyranet,
SpaceEEC and
vladfrangu
ckohen
force-pushed
the
feat/structures
branch
from
88f1614 to
0e58825
Compare
ckohen
force-pushed
the
feat/structures
branch
from
0e58825 to
3ea68e7
Compare
ckohen
force-pushed
the
feat/structures
branch
from
3ea68e7 to
29d67bf
Compare
|
⚡️ Lighthouse report for the changes in this PR:
Lighthouse ran on https://discord-js-git-fork-ckohen-feat-structures-discordjs.vercel.app/ |
ckohen
force-pushed
the
feat/structures
branch
2 times, most recently
from
e4cc324 to
98b83f1
Compare
| * The user's id | ||
| */ | ||
| public get id() { | ||
| return this[kData].id as 'id' extends Omitted ? never : APIUser['id']; |
There was a problem hiding this comment.
If we'll have to add this return type for each getter
- let's add it as the return type, if we can, else keep it as a cast
- Let's make a utility type for this (
NeverIfOmitted<Omitted, 'id', APIUser['id']>)
There was a problem hiding this comment.
Definitely utility type. I wish it didn't have to cast, but [value of thing] is not assignable to never (or undefined). Maybe we can add | never to all the types in Structure[kData] though and then it works? I'll try that, cause it'd make it a lot easier if it were a merged type.
Edit: still doesn't work :( (obviously not with never, but also not with partial)
There was a problem hiding this comment.
Can't there be a get method?
get<Key extends keyof Omit<Data, Omitted>>(property: Key) {
return this.data[property] as Key extends Omitted ? never : Data[Key];
}There was a problem hiding this comment.
Unfortunately not no. One, it's not as nice of an interface for consumers. Two, this doesn't allow us to convert from snake_case to camelCase.
I would love to do it this way, since it's a lot easier to maintain for us, but I think prioritizing the consumer interface is more important here. These base structures are a lot to implement at first, but shouldn't need to be touched super often once implemented.
There was a problem hiding this comment.
Think I got something better than all of this thanks to starting to write the utility type, fd45e9c implements this.
| } | ||
|
|
||
| public override _patch(data: Partial<APIExtendedInvite>) { | ||
| super._patch(data, { template: Invite.DataTemplate }); |
There was a problem hiding this comment.
Can't Structure class implement DataTemplate static property so that you don't need to pass it in super?
There was a problem hiding this comment.
static properties are a bit weird, especially in TS.
TS has no idea what this.constructor is, other than a function. This is technically correct, since this.constructor refers to whatever was constructed, not the current class.
I would love to write this as an abstract static property, but typescript really doesn't do that (yet).
I'm also not sure what the behavior in js would be if it were implemented on the base class and a consumer changed the value in a subclass like Message.DataTemplate = {}, and I'm not inclined to trust that even once I test it
ckohen
force-pushed
the
feat/structures
branch
3 times, most recently
from
1e9cf03 to
37ac617
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8417 +/- ##
==========================================
- Coverage 38.00% 37.85% -0.16%
==========================================
Files 239 246 +7
Lines 14894 15257 +363
Branches 1389 1422 +33
==========================================
+ Hits 5661 5775 +114
- Misses 9221 9470 +249
Partials 12 12
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| * | ||
| * @privateRemarks Overriden to `true` on `TextChannelMixin` | ||
| */ | ||
| public iSTextBased() { |
There was a problem hiding this comment.
| public iSTextBased() { | |
| public isTextBased() { |
ckohen
force-pushed
the
feat/structures
branch
2 times, most recently
from
fdbb176 to
1846ab6
Compare
refactor(Structure): patch to _patch
Co-authored-by: Synbulat Biishev <[email protected]>
Co-authored-by: Synbulat Biishev <[email protected]>
Co-authored-by: Qjuh <[email protected]>
ckohen
force-pushed
the
feat/structures
branch
from
1846ab6 to
19a307c
Compare
Please describe the changes this PR makes and why it should be merged:
Introduces a generic structure implementation for the main lib (and others) to build on.
TODO:
Membership ScreeningOh wait...this is still undocumented