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

BUG: Changes in component's attributes being reproduced in all the instances instead of just one #5199

Closed
2 tasks done
rmadeiraneto opened this issue Jun 23, 2023 · 2 comments

Comments

@rmadeiraneto
Copy link

GrapesJS version

  • I confirm to use the latest version of GrapesJS

What browser are you using?

Chrome v114

Reproducible demo link

https://jsfiddle.net/rmadeiraneto/t659usxv/39/

Describe the bug

How to reproduce the bug?

  1. Go to the blocks list and drag the custom component "example" to the canvas
  2. Three elements that are coming from a default attribute (array) are displayed in the canvas
  3. Select the "Example" component
  4. Go to the traits panel and click on the plus icon a couple of times
  5. This would add items to the array that is in the attributes of the Example component and draw them in the page
  6. Drag another instance of the Example block into the canvas
  7. Notice that the items that you added to the array are being also displayed in the new instance of the component

What is the expected behavior?
When creating the second instance of the same component after changing the attributes of the first one, it's expected that the attributes of the second component would be the first array defined in the defaults [1,2,3] under customArray prop, instead of the manipulated array [1,2,3,4,5].

What is the current behavior?
When we change the attributes of an instance of a component, since the reference to the default object is probably being preserved, we are also changing the defaults of the component, making our changes on a single component instance being reproduced in all instances of the same component. When GrapesJs assigns custom defaults to components' attributes, it needs to create a shallow copy of the objects. When making the same thing with strings or integers, instead of objects or arrays, the issue is not verified, that's why I think it is a matter of keeping the reference for the defaults' objects when assigning the component's attributes.

Code of Conduct

  • I agree to follow this project's Code of Conduct
@artf
Copy link
Member

artf commented Jul 4, 2023

Thanks @rmadeiraneto for the report. Yeah, unfortunately that's an issue if you're mutating arrays/objects properties in that way and to avoid that you have 2 options:

  1. Avoid direct mutations (assign new references when you have to update them)
  2. Define defaults as a function
defaults: () => ({ customArray: [1,2,3] }),

But unfortunately... the second options doesn't work right now 😅 I'll fix it in the next release

@artf artf closed this as completed in 8d7af1c Jul 4, 2023
artf added a commit that referenced this issue Jul 4, 2023
* Add autoFormat option to CodeMirrorEditor

* Add optsCodeViewer

* Remove unused options

* Use createViewer in ExportTemplate

* Up ExportTemplate

* Cleanup

* Up panel/index to TS

* Refactor Panels TS #5144

* Cleanup

* Up panels test

* Fix `usePlugin is not a function`. Closes #5167

* Move css_composer/index to TS

* Up css_composer tests

* Up tests

* Add `addStyles` option to `editor.Css.setRule`. Closes #5173

* Update setRule JSDoc

* Fix PropertyStack in bundled dts file. Closes #5154

* Fixed broken link in README.md (#5188)

Update README.md

added ".com" in a not functioning link.

* Refactor Traits Collection (#4983)

* Refactor Traits Collection

* Fix css prefix

* Fix trait undo and add test for it

---------

Co-authored-by: Artur Arseniev <[email protected]>

* Update keymaps add jsdoc

* Improve typings (#5192)

* add component:resize to ComponentEvent type

* limit storageManager type to be 'local' | 'remote' | undefined

* add type to components parameter

* Use LiteralUnion in storage_manager config

* Up editor TS

* Improve components TS

* Update ComponentModelDefinition

* [Docs] Update Broken Links for (Component Types, and Commands) (#5196)

update links

* Don't remove styles with avoidInlineStyle #4503

* Up block_manager

* Up device_manager

* Up pages TS

* Add PageProperties

* Update PageManager TS

* Export Sector/s in TS

* Up style_manager TS

* Up Property TS

* Up TS

* Up TS

* Up canvas class

* Fix SwitchVisibility

* Store custom selector manager container

* Refactor OpenStyleManager

* Up OpenStyleManager

* Up OpenStyleManager

* Up selector_manager

* Add custom option to trait_manager

* Up trait model

* Up OpenTraitManager for custom traits

* Up

* Handle properly Component model `defaults` as functions. Closes #5199

* Fix PropertyFactory tests

* Up RTE doc

* Update rich_text_editor.md (#5201)

The previous code snippet encountered a syntax error when attempting to use it in my application. To ensure its usability for others, I have made necessary updates to resolve the syntax error and enable successful execution. These modifications aim to provide a code snippet that can be easily utilized by anyone without encountering any syntax-related issues.

* Build

* Up docs

---------

Co-authored-by: pfaffmann <[email protected]>
Co-authored-by: Alex Ritter <[email protected]>
Co-authored-by: Julia Alberici <[email protected]>
Co-authored-by: Abdelrhman Said <[email protected]>
Co-authored-by: Ai Anshu <[email protected]>
@rmadeiraneto
Copy link
Author

@artf using defaults as a function works, thanks for quick response and for providing the alternative fix on this issue.
About the first suggestion, I think it's not about people should mutate or not the object, because we're talking about using the provided method (set). I'm sorry I didn't have the time to look at the source code, but you're probably mutating the defaults object inside the 'set' method, probably because the reference comes from early on in the component lifecycle. Again, without looking into the code, I assume that when you merge the defaults with the attributes of the component, you're keeping the reference to the defaults object instead of using a merge method that would return a copy instead, for example, the from immutable.
I'll try to find some time to look into the code and suggest something more specific

Again, thanks for the alternative and for this amazing library, keep up the great work!

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

No branches or pull requests

2 participants