fix: don't mutate user provided array

[imranbarbhuiya]

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

Calling embed.setFields twice will mutate the array passed in the first call. This PR fixes it.

I was going to change it in spliceFieldsonly but thought this issue might be present in other builders too so doing it in normalizeArray makes more sense

Status and versioning classification:

• 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

[vercel]

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)	Visit Preview		Apr 30, 2024 8:41pm
discord-js-guide	⬜️ Ignored (Inspect)	Visit Preview		Apr 30, 2024 8:41pm

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (a44ada6) 58.45% compared to head (ff61597) 58.45%.
Report is 3 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #10014   +/-   ##
=======================================
  Coverage   58.45%   58.45%           
=======================================
  Files         241      241           
  Lines       17130    17130           
  Branches     1241     1241           
=======================================
  Hits        10014    10014           
  Misses       7071     7071           
  Partials       45       45           

Flag	Coverage Δ
builders	95.53% <100.00%> (ø)
next	∅ <ø> (∅)

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[Jiralite]

Is it possible to write tests for this?

[Syjalo]

.addFields(), not .setFields()

const embed = new EmbedBuilder();
const fields1 = [{ name: '1', value: '1' }, { name: '2', value: '2' }, { name: '3', value: '3' }];
const fields2 = [{ name: '4', value: '4' }, { name: '5', value: '5' }, { name: '6', value: '6' }];

embed.addFields(fields1);
embed.addFields(fields2);

console.log(fields1);
/**
[
  { name: '1', value: '1' },
  { name: '2', value: '2' },
  { name: '3', value: '3' },
  { name: '4', value: '4' },
  { name: '5', value: '5' },
  { name: '6', value: '6' }
]
**/
console.log(fields2);
/**
[
  { name: '4', value: '4' },
  { name: '5', value: '5' },
  { name: '6', value: '6' }
]
**/

[vladfrangu]

This is an incomplete PR i'd say - it needs to also clone the array in the other case

[imranbarbhuiya]

This is an incomplete PR i'd say - it needs to also clone the array in the other case

all the methods uses normalizeArray so ig it covers all the cases?

[imranbarbhuiya]

.addFields(), not .setFields()

const embed = new EmbedBuilder();
const fields1 = [{ name: '1', value: '1' }, { name: '2', value: '2' }, { name: '3', value: '3' }];
const fields2 = [{ name: '4', value: '4' }, { name: '5', value: '5' }, { name: '6', value: '6' }];

embed.addFields(fields1);
embed.addFields(fields2);

console.log(fields1);
/**
[
  { name: '1', value: '1' },
  { name: '2', value: '2' },
  { name: '3', value: '3' },
  { name: '4', value: '4' },
  { name: '5', value: '5' },
  { name: '6', value: '6' }
]
**/
console.log(fields2);
/**
[
  { name: '4', value: '4' },
  { name: '5', value: '5' },
  { name: '6', value: '6' }
]
**/

calling setFields twice or using addFields all has this issue but this pr should fix both cases

[imranbarbhuiya]

Is it possible to write tests for this?

ok

[Jiralite]

I asked for tests because I couldn't reproduce this via setFields(), by after @Syjalo's comment, I was able to reproduce with addFields(). Seeing the tests should be fairly obvious as to what the expected and current behaviour is!

[imranbarbhuiya]

I asked for tests because I couldn't reproduce this via setFields(), by after @Syjalo's comment, I was able to reproduce with addFields(). Seeing the tests should be fairly obvious as to what the expected and current behaviour is!

it happens when u use setFields first with a, run again with a new value, log a and it's mutated. I added test for normalizeArray to return cloned array, should I also add for embed fields?

[Jiralite]

it happens when u use setFields first with a, run again with a new value, log a and it's mutated.

Do you mind sharing a code sample—not sure I'm getting it.

[imranbarbhuiya]

it happens when u use setFields first with a, run again with a new value, log a and it's mutated.

Do you mind sharing a code sample—not sure I'm getting it.

I just tried and then checked my original code and yeah it was addFields then setFields, sorry for the confusion

[almeidx]

More often than not, the output of this function is already going to be cloned (.map(), spread, etc.). Perhaps we should instead fix the Embed.addFields() method, which seems to be the only place where the value is not being cloned.

However, if you choose to keep it as it is, we should remove this clone:

discord.js/packages/builders/src/components/selectMenu/MentionableSelectMenu.ts

Line 113 in 54453b0

this.data.default_values = normalizedValues.slice();

[imranbarbhuiya]

More often than not, the output of this function is already going to be cloned (.map(), spread, etc.). Perhaps we should instead fix the Embed.addFields() method, which seems to be the only place where the value is not being cloned.

However, if you choose to keep it as it is, we should remove this clone:

discord.js/packages/builders/src/components/selectMenu/MentionableSelectMenu.ts

Line 113 in 54453b0

this.data.default_values = normalizedValues.slice();

Can i get a core members feedback on which one to prefer here?