Skip to content

Conversation

@wkillerud
Copy link
Contributor

@wkillerud wkillerud commented Jul 1, 2025

Demonstrating an issue that blocks us from adopting the native structuredClone as a replacement for lodash.clonedeep in client.

This should work, but doesn't. The css and js arrays become a list of empty objects.

- outgoing.manifest = clonedeep(cached);
+ outgoing.manifest = structuredClone(cached);

I think cloneDeep only works because it doesn't actualy clone the asset class instances.

https://github.com/lodash/lodash/blob/ddfd9b11a0126db2302cb70ec9973b66baec0975/lodash.js#L2657

We hit this branch where it is an object, but not a clonable one, so it "clones" by returning the same instance. Confirmed with console log.

console.log(outgoing.manifest.css[0] === cached.css[0]);

This on the line after cloning prints true across all unit tests in client.

@wkillerud
Copy link
Contributor Author

wkillerud commented Jul 1, 2025

Maybe since cloneDeep is actually a shallow copy in our case, a simple spread could be all we need. But it's unexpected.

wkillerud added a commit to podium-lib/client that referenced this pull request Jul 1, 2025
This replacement with a shallow clone might look odd, but only one
value at the root level is mutated. Also, because of a quirk with
how clonedeep works with the AssetCss and AssetJs classes in
podium utils, the entries in the css and js arrays weren't actually
cloned – instead the values were copied.

See podium-lib/utils#289 for a more thorough
explanation with links to code.
@wkillerud wkillerud closed this Jul 1, 2025
wkillerud added a commit to podium-lib/client that referenced this pull request Jul 1, 2025
This replacement with a shallow clone might look odd, but only one
value at the root level is mutated. Also, because of a quirk with
how clonedeep works with the AssetCss and AssetJs classes in
podium utils, the entries in the css and js arrays weren't actually
cloned – instead the values were copied.

See podium-lib/utils#289 for a more thorough
explanation with links to code.
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.

2 participants