Skip to content
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

fix(User): set User#bot to false if not partial #4706

Merged
merged 2 commits into from
Aug 15, 2020
Merged

fix(User): set User#bot to false if not partial #4706

merged 2 commits into from
Aug 15, 2020

Conversation

vaporoxx
Copy link
Contributor

Please describe the changes this PR makes and why it should be merged:

#4636 recently introduced a bug where User#bot was set to null instead of false. This is the case because it checked if the bot property was present in the data to see if it was a partial or not, however this property also does not exist for a regular user. This has been fixed by using User#partial which checks if the username property exists instead.

Status

  • Code changes have been tested against the Discord API, or there are no code changes
  • I know how to update typings and have done so, or typings don't need updating

Semantic versioning classification:

  • This PR changes the library's interface (methods or parameters added)
    • This PR includes breaking changes (methods removed or renamed, parameters moved or removed)
  • This PR only includes non-code changes, like changes to documentation, README, etc.

@advaith1
Copy link
Contributor

It's intended to be null, it being false was the bug. this pr undos the bug fix

@vaporoxx
Copy link
Contributor Author

vaporoxx commented Aug 15, 2020

Yes, but the current bug is that it's set to null even if the user is not partial instead of false which would be expected. With this PR, partial users will still have null as the value for their .bot property but full User objects won't

@tipakA
Copy link
Contributor

tipakA commented Aug 15, 2020

But it's very similar in behavior to how API doesn't send TextChannel.nsfw initially, yet library sets to false if it is not explicitly in the payload, as was done in #4593

@almostSouji
Copy link
Member

almostSouji commented Aug 15, 2020

original

It's intended to be null, it being false was the bug. this pr undos the bug fix

No, that's a misconception. The bug was introduced with the "bug fix".
If Discord does not send a field in the initial data payload which is documented to be a boolean it has to be considered false. I applied a fix to this after discussion on DAPI in #4593. The exact same principle applies to User#bot.

(I even mentioned exactly that point in my PR referenced above under "further considerations")

Now, this will lead to inconsistencies with partials, since we might falsely declare bots which we only have a partial User for as "false", however that's a caveat to swallow, imo and is regulated with "partials should be considered defunct before fetching". since the fetch then will retrieve and update the truthful true for the partial (but now completed) bot.

Has discord documented that anywhere? no
Is that majorly annoying, since we need to monkeypatch the API not sending data fields? yes
Should we still do it? probably

After some more internal discussion: discord not documenting this behavior is an absolute mess and handling it ourselves even more of an headache.

@advaith1
Copy link
Contributor

advaith1 commented Aug 15, 2020

Ah, I see, I assumed that it was intentional to be consistent with the API from #4702 (comment)
well we should do the same thing for User#system

src/structures/User.js Outdated Show resolved Hide resolved
@NotSugden NotSugden mentioned this pull request Aug 15, 2020
5 tasks
@iCrawl iCrawl merged commit db512d8 into discordjs:master Aug 15, 2020
@louisglen1
Copy link
Contributor

The JSdoc for User#bot has not been updated to account for the changes in this PR.
Unrelated to this PR, however the same issue appears to be apparent in the JSdoc for User#system.

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

Successfully merging this pull request may close these issues.

12.3.0 broke the User.bot property to only be null | true not false | true like in 12.2.0
6 participants