fix: don't mutate user provided array #10014

imranbarbhuiya · 2023-12-02T12:26:40Z

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 · 2023-12-02T12:26:44Z

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		Dec 2, 2023 7:15pm
discord-js-guide	⬜️ Ignored (Inspect)	Visit Preview		Dec 2, 2023 7:15pm

codecov · 2023-12-02T12:29:49Z

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 · 2023-12-02T15:20:39Z

Is it possible to write tests for this?

Syjalo · 2023-12-02T17:26:04Z

.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 · 2023-12-02T18:10:05Z

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

imranbarbhuiya · 2023-12-02T18:59:11Z

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 · 2023-12-02T18:59:41Z

.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 · 2023-12-02T18:59:50Z

Is it possible to write tests for this?

ok

Jiralite · 2023-12-02T19:03:11Z

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 · 2023-12-02T19:09:22Z

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?

.vscode/settings.json

Jiralite · 2023-12-02T19:13:39Z

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 · 2023-12-02T19:22:22Z

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();

vladfrangu · 2023-12-03T22:20:46Z

packages/builders/__tests__/util.test.ts

+		expect(normalizeArray([1, 2, 3])).toEqual([1, 2, 3]);
+	});
+
+	test('always returns a clone', () => {


Add a test case for rest too

Rest doesnt gets cloned in normalizeArray as the original input wasn't an array but rest params

imranbarbhuiya · 2023-12-04T02:44:01Z

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?

kyranet · 2023-12-24T16:11:50Z

packages/builders/__tests__/util.test.ts

+
+	test('always returns a clone', () => {
+		const arr = [1, 2, 3];
+		expect(normalizeArray([arr])).not.toBe(arr);


I think this should check toEqual(arr) to make sure the output is the same as the input followed by the not.toBe(arr) to signal that the arrays are equal but they're not the same instance.

fix(builders): don't mutate user provided array

95d44ad

imranbarbhuiya requested a review from a team as a code owner December 2, 2023 12:26

imranbarbhuiya requested review from vladfrangu, iCrawl, kyranet and Jiralite December 2, 2023 12:26

github-actions bot added the packages:builders label Dec 2, 2023

Jiralite added this to the builders 1.8.0 milestone Dec 2, 2023

Jiralite added the semver:patch label Dec 2, 2023

Jiralite changed the title ~~fix(builders): don't mutate user provided array~~ fix: don't mutate user provided array Dec 2, 2023

test: add normalize array tests

022ada8

Jiralite reviewed Dec 2, 2023

View reviewed changes

.vscode/settings.json Outdated Show resolved Hide resolved

chore: revert vscode autochange

ff61597

almeidx requested changes Dec 2, 2023

View reviewed changes

vladfrangu requested changes Dec 3, 2023

View reviewed changes

baby636 mentioned this pull request Dec 20, 2023

[Snyk] Fix for 4 vulnerabilities baby636/discord.js#21

Open

kyranet reviewed Dec 24, 2023

View reviewed changes

fix: don't mutate user provided array #10014

fix: don't mutate user provided array #10014

imranbarbhuiya commented Dec 2, 2023 •

edited

vercel bot commented Dec 2, 2023 •

edited

codecov bot commented Dec 2, 2023 •

edited

Jiralite commented Dec 2, 2023 •

edited

Syjalo commented Dec 2, 2023 •

edited

vladfrangu commented Dec 2, 2023

imranbarbhuiya commented Dec 2, 2023

imranbarbhuiya commented Dec 2, 2023

imranbarbhuiya commented Dec 2, 2023

Jiralite commented Dec 2, 2023

imranbarbhuiya commented Dec 2, 2023 •

edited

Jiralite commented Dec 2, 2023

imranbarbhuiya commented Dec 2, 2023

almeidx left a comment

vladfrangu Dec 3, 2023

imranbarbhuiya Dec 4, 2023

imranbarbhuiya commented Dec 4, 2023

kyranet Dec 24, 2023

fix: don't mutate user provided array #10014

Are you sure you want to change the base?

fix: don't mutate user provided array #10014

Conversation

imranbarbhuiya commented Dec 2, 2023 • edited

vercel bot commented Dec 2, 2023 • edited

codecov bot commented Dec 2, 2023 • edited

Codecov Report

Jiralite commented Dec 2, 2023 • edited

Syjalo commented Dec 2, 2023 • edited

vladfrangu commented Dec 2, 2023

imranbarbhuiya commented Dec 2, 2023

imranbarbhuiya commented Dec 2, 2023

imranbarbhuiya commented Dec 2, 2023

Jiralite commented Dec 2, 2023

imranbarbhuiya commented Dec 2, 2023 • edited

Jiralite commented Dec 2, 2023

imranbarbhuiya commented Dec 2, 2023

almeidx left a comment

Choose a reason for hiding this comment

vladfrangu Dec 3, 2023

Choose a reason for hiding this comment

imranbarbhuiya Dec 4, 2023

Choose a reason for hiding this comment

imranbarbhuiya commented Dec 4, 2023

kyranet Dec 24, 2023

Choose a reason for hiding this comment

imranbarbhuiya commented Dec 2, 2023 •

edited

vercel bot commented Dec 2, 2023 •

edited

codecov bot commented Dec 2, 2023 •

edited

Jiralite commented Dec 2, 2023 •

edited

Syjalo commented Dec 2, 2023 •

edited

imranbarbhuiya commented Dec 2, 2023 •

edited