discord.js

discord.js copied to clipboard

Please describe the changes this PR makes and why it should be merged: Resolves #8647 Status and versioning classification:

• This PR changes the library's interface (methods or parameters added)

The latest updates on your projects. Learn more about Vercel for Git ↗︎

2 Ignored Deployments

Name	Status	Preview	Comments	Updated (UTC)
discord-js	⬜️ Ignored (Inspect)			Jul 7, 2023 9:50pm
discord-js-guide	⬜️ Ignored (Inspect)	Visit Preview		Jul 7, 2023 9:50pm

Codecov Report

Merging #8682 (adc0fc0) into main (75d91b5) will increase coverage by 27.40%. The diff coverage is 75.00%.

:exclamation: Current head adc0fc0 differs from pull request most recent head 5fb3382. Consider uploading reports for the commit 5fb3382 to get more accurate results

@@             Coverage Diff             @@
##             main    #8682       +/-   ##
===========================================
+ Coverage   58.20%   85.60%   +27.40%     
===========================================
  Files         227       96      -131     
  Lines       14902     9467     -5435     
  Branches     1131     1134        +3     
===========================================
- Hits         8673     8104      -569     
+ Misses       6189     1321     -4868     
- Partials       40       42        +2     

Flag	Coverage Δ
brokers	65.24% <ø> (+2.07%)	:arrow_up:
builders	98.59% <75.00%> (-0.10%)	:arrow_down:
collection	100.00% <ø> (+0.12%)	:arrow_up:
guide	?
next	?
proxy	81.53% <ø> (+5.22%)	:arrow_up:
rest	91.97% <ø> (-1.16%)	:arrow_down:
util	100.00% <ø> (+1.50%)	:arrow_up:
utilities	100.00% <ø> (ø)
voice	63.70% <ø> (+0.23%)	:arrow_up:
website	?
ws	59.83% <ø> (+5.97%)	:arrow_up:

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
packages/builders/src/messages/embed/Embed.ts	99.32% <75.00%> (-0.68%)	:arrow_down:

... and 198 files with indirect coverage changes

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

Could I get a review on this?

Wouldn't it be fine if users just do:

import { embedLength } from '@discordjs/builders';

embedLength(YourEmbedBuilderInstance.data);

?

Wouldn't it be fine if users just do

Yes, but why does Embed have a length property then?

I'm against this change, it goes against RFC about exposing a builder's data via getters:

Well I changed my mind, we should instead use the data field, however no getters should be added pointing to said field.

Furthermore, you can do as @iCrawl suggested. And regarding the main library's builder having the length getter, it's because it has all the other getters (footer, image, ...) as well as utilities (hexColor).

⚡️ Lighthouse report for the changes in this PR:

Category	Score
🟠 Performance	61
🟢 Accessibility	98
🟢 Best practices	100
🟢 SEO	94
🟠 PWA	70

Lighthouse ran on https://discord-js-guide-git-fork-redguy12-gh8647-discordjs.vercel.app/guide/home/introduction