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 Util#splitMessage when destructured #3456

Merged
merged 1 commit into from Sep 3, 2019

Conversation

@BannerBomb
Copy link
Contributor

commented Sep 1, 2019

Please describe the changes this PR makes and why it should be merged:
this PR fixes the Utils#splitMessage method because resolveString should be called as Util.resolveString since it's a static method.


Update
kyranet posted a better reason why this PR should be merged here.

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.
fixed splitMessage error
resolveString should be called as `Util.resolveString` since it's a static method.
@kyranet
kyranet approved these changes Sep 1, 2019
@Monbrey

This comment has been minimized.

Copy link
Contributor

commented Sep 1, 2019

What's the point of this exactly? The method wasn't broken, and this refers to Util already, as per MDN

this would be undefined if the method is deconstructed from the class, so I now see the use case for this

@Monbrey
Monbrey approved these changes Sep 1, 2019
@kyranet

This comment has been minimized.

Copy link
Contributor

commented Sep 1, 2019

I have to note, though, your reason for the fix is incorrect. The method works fine with this, it refers to the class itself (aka the Util class) as it's in a static method, as well as if it was an instance method, it would refer to the instance itself.

What breaks this is when you destructure the method, as in:

const { util: { splitMessage } } = require('discord.js');
splitMessage('');
// TypeError: Cannot read property 'resolveString' of undefined

But if you do not destructure the method itself, it will work just fine:

const { util } = require('discord.js');
util.splitMessage('');
// [ '' ]

That's because when destructuring the method, you're removing it's context, used for the this value, hence the error. I believe this PR should be merged as this problem makes the method inconsistent with all the others, which don't error when destructured.

@Gawdl3y Gawdl3y changed the title fixed Util#splitMessage error Fix Util#splitMessage when destructured Sep 3, 2019

@Gawdl3y
Gawdl3y approved these changes Sep 3, 2019

@iCrawl iCrawl merged commit 5d95a4b into discordjs:master Sep 3, 2019

3 checks passed

ESLint
Details
typings-lint
Details
docs-lint
Details

@BannerBomb BannerBomb deleted the BannerBomb:patch-1 branch Sep 3, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.