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

A suggestion to drop lodash.clonedeep in favour of structuredClone #503

Closed
StefanNedelchev opened this issue Mar 22, 2023 · 4 comments · Fixed by #517
Closed

A suggestion to drop lodash.clonedeep in favour of structuredClone #503

StefanNedelchev opened this issue Mar 22, 2023 · 4 comments · Fixed by #517

Comments

@StefanNedelchev
Copy link
Contributor

StefanNedelchev commented Mar 22, 2023

When I import @cornerstonejs/core into an Angular project and try to build it I get the following warning:
image

This is a common warning that appears if pretty much any lodash feature is used (or other commonjs dependencies) and there is a good reason behind. But even if we put Angular aside, it's always a good thing if we can get rid of 3rd party packages when there is an opportunity. Since Cornerstone 3D is meant to be for modern browsers anyway, we could use the structuredClone() method for deep cloning of objects instead of relying on lodash.clonedeep(). The function is supported by pretty much every modern desktop and mobile browser with the exception that it's not available in workers on Opera and Safari.

@sedghi
Copy link
Member

sedghi commented Mar 23, 2023

Yes indeed, I prefer structuredClone as well, if you can create a PR I would be happy to review and merge

@StefanNedelchev
Copy link
Contributor Author

StefanNedelchev commented Mar 24, 2023

@sedghi I created a PR #517. Although the builds and tests passed locally, the tests here failed. From the logs I see there is one of the errors:

 An error was thrown in afterAll
  DataCloneError: Failed to execute 'structuredClone' on 'Window': SVGCircleElement object could not be cloned.
  error properties: Object({ code: 25, INDEX_SIZE_ERR: 1, DOMSTRING_SIZE_ERR: 2, HIERARCHY_REQUEST_ERR: 3, WRONG_DOCUMENT_ERR: 4, INVALID_CHARACTER_ERR: 5, NO_DATA_ALLOWED_ERR: 6, NO_MODIFICATION_ALLOWED_ERR: 7, NOT_FOUND_ERR: 8, NOT_SUPPORTED_ERR: 9, INUSE_ATTRIBUTE_ERR: 10, INVALID_STATE_ERR: 11, SYNTAX_ERR: 12, INVALID_MODIFICATION_ERR: 13, NAMESPACE_ERR: 14, INVALID_ACCESS_ERR: 15, VALIDATION_ERR: 16, TYPE_MISMATCH_ERR: 17, SECURITY_ERR: 18, NETWORK_ERR: 19, ABORT_ERR: 20, URL_MISMATCH_ERR: 21, QUOTA_EXCEEDED_ERR: 22, TIMEOUT_ERR: 23, INVALID_NODE_TYPE_ERR: 24, DATA_CLONE_ERR: 25 })
  Error: Failed to execute 'structuredClone' on 'Window': SVGCircleElement object could not be cloned.
      at resetCornerstoneToolsState (webpack-internal:///./packages/tools/src/store/state.ts:8:4094)
      at Module.destroy (webpack-internal:///./packages/tools/src/init.ts:21:14870)
      at UserContext.eval (webpack-internal:///./packages/tools/test/FrameOfReferenceSpecificToolStateManager_test.js:56:45)
      at <Jasmine>

I double checked the MDN docs for structuredClone() and by looking at supported types it seems that HTML and SVG elements are not present. A quick test in the a JS console confirmed that:
image

I'm leaving the draft PR open in case if anyone wants to investigate different solutions to these cases.

bionoone pushed a commit to bionoone/cornerstone3D-beta that referenced this issue Mar 27, 2023
@StefanNedelchev
Copy link
Contributor Author

@sedghi maybe we can close this since #517 has been merged?

@sedghi
Copy link
Member

sedghi commented Oct 18, 2023

Yes, thanks

@sedghi sedghi closed this as completed Oct 18, 2023
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 a pull request may close this issue.

2 participants