Prevent cloning objects into previousData that are not plain objects #2939
Conversation
dmarcos
force-pushed
the
avoidCloningNonObjects
branch
from
3724c4f to
ef75dc2
Compare
dmarcos
force-pushed
the
avoidCloningNonObjects
branch
2 times, most recently
from
fb6c826 to
2bd9451
Compare
|
Can you paste pseudocode that describes " |
src/core/component.js
Outdated
| data = typeof schema.default === 'object' ? utils.clone(schema.default) : schema.default; | ||
| // Clone default value if plain object so components don't share the same object | ||
| // that might be modified by the user. | ||
| data = typeof schema.default === 'object' && schema.default.constructor === Object ? utils.clone(schema.default) : schema.default; |
There was a problem hiding this comment.
Do we need both typeof and .constructor checks?
| */ | ||
| function cloneData (data) { | ||
| var clone = {}; | ||
| var parsedProperty; |
There was a problem hiding this comment.
declare vars up top.
i wonder if we should start to use for (key in obj) for every loop over an object where we know it's a plain JS object. e.g., change some of the code in the buildData to also use for...in
|
An example is: |
dmarcos
force-pushed
the
avoidCloningNonObjects
branch
from
2bd9451 to
7212794
Compare
|
@fernandojsg is this true that this blocks aframevr/a-saturday-night#118 (comment) ? |
|
@cvan yes, it was described on my PR aframevr/a-saturday-night#117 (I closed it because @caseyyee and I were doing practically the same) |
|
Speaking towards the technicals, I start to often use strings rather than |
default values that are not plain objects into data.
dmarcos
force-pushed
the
avoidCloningNonObjects
branch
from
7212794 to
a6fa87c
Compare
|
I addressed the review comments and fixed the test. Ok to merge? |
| data = typeof previousData === 'object' ? utils.clone(previousData) : {}; | ||
| // Clone previous data to prevent sharing references with attrValue that might be | ||
| // modified by the user. | ||
| data = typeof previousData === 'object' ? cloneData(previousData) : {}; |
There was a problem hiding this comment.
I believe you can use utils.extend({}, previousData). The problem with utils.clone is it uses JSON to do it. Getting rid of the clone function you made.
There was a problem hiding this comment.
utils.extend copies references. I still want to clone the plain object properties
|
r+wc |
4 participants
a-saturday-nightusesselectorproperties that change at run time. This triggeredConverted circular structure to JSONerrors when trying to clone into previousData A-Frame entity elements that hold references to other A-Frame entities via components .We should only be cloning plain objects.