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

Avoid unwanted mutation to passed objects array to Group constructor #9151

Merged
merged 3 commits into from
Aug 18, 2023

Conversation

jiayihu
Copy link
Contributor

@jiayihu jiayihu commented Aug 11, 2023

Minor fix: since Collection mutates the _objects array, this can cause really hard-to-debug bugs if the passed array of objects is used for other purposes as well by the application. To avoid any complication for the unaware developer, a copy should be made.

asturur
asturur previously approved these changes Aug 13, 2023
CHANGELOG.md Outdated Show resolved Hide resolved
@asturur
Copy link
Member

asturur commented Aug 13, 2023

maybe a dumb unit test , an new group JEST file, that tests that given an array and doing a collection mutation on it, the original array is still intact, and that the 2 arrays do not pass the strict equality test.

If we wanted to go full mutation protection we should clone the object, that would make the constructor async and i don't think we should do it at all.

@jiayihu jiayihu changed the title Avoid unwanted mutation in Group Avoid unwanted mutation to passed objects array to Group constructor Aug 13, 2023
@jiayihu
Copy link
Contributor Author

jiayihu commented Aug 13, 2023

Done. Renamed changelog text and added a Jest test.

Copy link
Contributor

@ShaMan123 ShaMan123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the comment can be simpler but it is not a deal breaker

src/shapes/Group.ts Show resolved Hide resolved
@ShaMan123
Copy link
Contributor

ShaMan123 commented Aug 14, 2023

@asturur can you look into giving the workflows permissions so that forks will be able to pass all tests?
I remember I opened an issue for it #8336

@asturur asturur merged commit b0dba07 into fabricjs:master Aug 18, 2023
@jiayihu jiayihu deleted the fix/group-mutation branch August 18, 2023 14:10
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.

3 participants