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

feat(structuredClone): drop lodash.clonedeep in favor of structuredClone #517

Merged
merged 9 commits into from
Oct 16, 2023
Merged

feat(structuredClone): drop lodash.clonedeep in favor of structuredClone #517

merged 9 commits into from
Oct 16, 2023

Conversation

StefanNedelchev
Copy link
Contributor

Resolves #503

Notes:

  • @types/node had to be updated to v18 because the minimal version of the type definitions that support structuredClone is 17.
  • typescript had to be updated as well to a version that supports structuredClone
  • the lib option in tsconfig was set to ES2021 instead of ES2022 because with ES2022 I'm getting errors during build:update-api (any help with that is welcome)
  • the type assertion in Settings.ts had to be done due to TypeScript error after updating to the newer version
  • the type corrections in createLabelmapVolumeForViewport.ts had to be done because the use of structuredClone type checks the input (unlike lodash.clonedeep)

@netlify
Copy link

netlify bot commented Mar 24, 2023

Deploy Preview for cornerstone-3d-docs ready!

Name Link
🔨 Latest commit d532d90
🔍 Latest deploy log https://app.netlify.com/sites/cornerstone-3d-docs/deploys/64721d7037cab700081d310d
😎 Deploy Preview https://deploy-preview-517--cornerstone-3d-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@StefanNedelchev
Copy link
Contributor Author

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.

@wayfarer3130
Copy link
Collaborator

@StefanNedelchev - could you add the transfer option with a reference to the SVG object contained in it? I believe that should allow removing the SVG object from the old spot and creating it in the new spot.
Also, please see if you can merge: #514 to include this.

@StefanNedelchev
Copy link
Contributor Author

@StefanNedelchev - could you add the transfer option with a reference to the SVG object contained in it? I believe that should allow removing the SVG object from the old spot and creating it in the new spot. Also, please see if you can merge: #514 to include this.
I was thinking about adding the transfer options, I will try that. #514 is supposed to be ready to merge but I don't seem to have the merge button

@StefanNedelchev StefanNedelchev marked this pull request as ready for review March 28, 2023 20:38
@@ -37,7 +37,7 @@
"packages/**/dist",
"packages/**/lib",
"packages/**/lib-esm",
"packages/adapters",
"packages/adapters/**/*.js",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had to do this because of a new error that started appearing after the latest sync with main:

Parsing error: ESLint was configured to run on `<tsconfigRootDir>/packages/adapters/src/adapters/Cornerstone3D/EllipticalROI.ts` using `parserOptions.project`: <tsconfigRootDir>/tsconfig.json

@StefanNedelchev
Copy link
Contributor Author

@wayfarer3130 check the solution I did with the structuredClone in the sate store. Passing the svg elements to the transferable items didn't work (it wasn't even accepted because it's for binary objects) so I had to use this funky solution of excluding the element when passing it to the structuredClone method and additionally copying the svgElements map with a spread operator.

@StefanNedelchev
Copy link
Contributor Author

@sedghi sorry for leaving this PR behind. I took some time to sync it with the latest cornerstone 3d and remove some leftovers of lodash.cloneDeep. The CI seems successful now but I have to point out that for some reason I had to add 2 rules in the ESLint ignore configuration due to questionable lint errors ("ESLint was configured to run ... However, that TSConfig does not include this file") when trying to commit.

@sedghi sedghi changed the base branch from main to beta October 16, 2023 18:16
@sedghi sedghi changed the title drop lodash.clonedeep in favor of structuredClone feat(structuredClone): drop lodash.clonedeep in favor of structuredClone Oct 16, 2023
@sedghi sedghi merged commit e562d72 into cornerstonejs:beta Oct 16, 2023
5 checks passed
sedghi pushed a commit that referenced this pull request Apr 9, 2024
* chore(beta): release beta for v2

* chore(beta): release beta for v2 (#715)

* chore(beta): release beta for v2 again (#716)

* chore(version): version.json [skip ci]

* chore(version): Update package versions [skip ci]

* fix(camera initial zoom): make the image fit the canvas for initial reset

* fix: Render to pixel exact positioning, and add a zoom setting

* api-check

* PR updates - mostly docs and some cleanup

* fix: Offset positions sometimes render moire patterns

* fix: Fractional positioning for center point

* Fix the center offset

* PR review comments

* API check

* chore(version): version.json [skip ci]

* chore(version): Update package versions [skip ci]

* chore(version): version.json [skip ci]

* chore(version): Update package versions [skip ci]

* feat(structuredClone): drop lodash.clonedeep in favor of structuredClone (#517)

* drop lodash.clonedeep in favor of structuredClone

* copy svgNodeCache outside structured clone when resetting state

* remove  clonedeep leftovers

* api

---------

* chore(version): version.json [skip ci]

* chore(version): Update package versions [skip ci]

* api

* new stack images

* Revert changes

* Add synchronizer and test configuration for scale

* Fix positioning

* fix zoom

* Fix pan for scale to fit

* Fixes for resize

* Changes to make tests pass again

* Fix scale display

* Fixes for resize viewport

* Fix centering/positioning without needing initial camera setting

* Fix resize issues with rotation

* Cleanup for build

* Add todo for fuzzy fit to viewport scaling issue

* Cleanup of types/values

* fix: render annotations without plane normal on viewports and update default svg colors (#1159)

* chore(version): version.json [skip ci]

* chore(version): Update package versions [skip ci]

* fix(cine): playclip to allow loop config (#1163)

* chore(version): version.json [skip ci]

* chore(version): Update package versions [skip ci]

* fix(DicomImageLoader): Returns options.loader (#1154)

* chore(version): version.json [skip ci]

* chore(version): Update package versions [skip ci]

* docs: Update getAnnotation Api (#1169)

annotations' S word should be removed and getAnnotations method parameters are written in reverse order

* chore(version): version.json [skip ci]

* chore(version): Update package versions [skip ci]

* examples: Add better helper method for adding tools dropdown (#1150)

* test: Add better helper method for adding tools dropdown.

* Add stack annotation example

* Add labelmap tools as well

* Add contour tools import as well.

* PR review comment fixes

* api-check

* chore(version): version.json [skip ci]

* chore(version): Update package versions [skip ci]

* Make the size work for positioning

* Fix ES6 change

* update

* Add color utility module to core package

* Add color utility module to core package

* revert structured clone

* Fix cloning of lastSliceAnnotation in ContourInterpolation_test.js

* Fix lodash import in addSegmentations.ts

* PR comments
sedghi pushed a commit that referenced this pull request Jun 11, 2024
…one (#517)

* drop lodash.clonedeep in favor of structuredClone

* copy svgNodeCache outside structured clone when resetting state

* remove  clonedeep leftovers

* api

---------
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.

A suggestion to drop lodash.clonedeep in favour of structuredClone
3 participants