cornerstone3D icon indicating copy to clipboard operation
cornerstone3D copied to clipboard

Try Remove Cicular Deps

Open salimkanoun opened this issue 2 years ago • 11 comments

fixes https://github.com/cornerstonejs/cornerstone3D/issues/742 Continuing the discussion started herer : https://github.com/cornerstonejs/cornerstone3D/issues/742

The problem i found in that many file import module using the root index, especially types.

So as the index indexes almost everything, the index import modules that imports types using the index creating cicular deps.

I tried to start removing them, but there's a lot of circular deps

What do you think, should i continue or is some reachitecturing import neede?

salimkanoun avatar Sep 24 '23 14:09 salimkanoun

Deploy Preview for cornerstone-3d-docs ready!

Name Link
Latest commit 2974a991a9db646199fd778e2cd0a326fba794e5
Latest deploy log https://app.netlify.com/sites/cornerstone-3d-docs/deploys/65162ba06a6a5b0008568973
Deploy Preview https://deploy-preview-798--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 configuration.

netlify[bot] avatar Sep 24 '23 14:09 netlify[bot]

so where this happens? during yarn install?

sedghi avatar Sep 25 '23 15:09 sedghi

when building cornerstone in a project with vite there a lot of circular deps warning, and the tools are not working on the built project. I checked the circular deps with dpdm to list them

salimkanoun avatar Sep 25 '23 16:09 salimkanoun

I tried to fix all the types with eslint, probably that is good enough for majority of the issues, though for some reason dpdm shows a lot more than madge (https://www.npmjs.com/package/madge) which seems to be a lot more used.

I added a script to packages called dependency-test so you can run it via yarn run dependency-test

After fixing the type imports via import type

  • I fixed the two non-type circular dependency in core -> madge pass
  • There are 5 circular dependency in tools that I haven't figure out a way to fix it, seems to be more complex and require some thinking for refactoring
1) stateManagement/annotation/annotationSelection.ts > stateManagement/annotation/annotationState.ts > stateManagement/annotation/FrameOfReferenceSpecificAnnotationManager.ts > stateManagement/annotation/annotationVisibility.ts
2) stateManagement/annotation/annotationState.ts > stateManagement/annotation/FrameOfReferenceSpecificAnnotationManager.ts > stateManagement/annotation/annotationVisibility.ts
3) stateManagement/segmentation/segmentationState.ts > stateManagement/segmentation/triggerSegmentationEvents.ts
4) store/index.ts > store/removeEnabledElement.ts > store/SynchronizerManager/getSynchronizersForViewport.ts
5) store/index.ts > store/removeEnabledElement.ts > store/ToolGroupManager/getToolGroupForViewport.ts
  • streaming loader pass
  • dicom image loader has 12 errors
1) externalModules.ts > imageLoader/registerLoaders.ts > imageLoader/wadors/index.ts > imageLoader/wadors/getPixelData.ts > imageLoader/internal/index.ts > imageLoader/internal/xhrRequest.ts
2) externalModules.ts > imageLoader/registerLoaders.ts > imageLoader/wadors/index.ts > imageLoader/wadors/loadImage.ts
3) externalModules.ts > imageLoader/registerLoaders.ts > imageLoader/wadors/index.ts > imageLoader/wadors/loadImage.ts > imageLoader/createImage.ts
4) externalModules.ts > imageLoader/registerLoaders.ts > imageLoader/wadors/index.ts > imageLoader/wadors/loadImage.ts > imageLoader/createImage.ts > imageLoader/convertColorSpace.ts > imageLoader/colorSpaceConverters/index.ts > imageLoader/colorSpaceConverters/convertPALETTECOLOR.ts
5) externalModules.ts > imageLoader/registerLoaders.ts > imageLoader/wadors/index.ts > imageLoader/wadors/loadImage.ts > imageLoader/createImage.ts > imageLoader/getImageFrame.ts
6) externalModules.ts > imageLoader/registerLoaders.ts > imageLoader/wadors/index.ts > imageLoader/wadors/metaData/index.ts > imageLoader/wadors/metaData/metaDataProvider.ts
7) imageLoader/wadors/metaDataManager.ts > imageLoader/wadors/retrieveMultiframeMetadata.ts
8) externalModules.ts > imageLoader/registerLoaders.ts > imageLoader/wadouri/index.ts > imageLoader/wadouri/dataSetCacheManager.ts
9) externalModules.ts > imageLoader/registerLoaders.ts > imageLoader/wadouri/index.ts > imageLoader/wadouri/dataSetCacheManager.ts > imageLoader/wadouri/dataset-from-partial-content.ts
10) imageLoader/wadouri/dataSetCacheManager.ts > imageLoader/wadouri/retrieveMultiframeDataset.ts
11) externalModules.ts > imageLoader/registerLoaders.ts > imageLoader/wadouri/index.ts > imageLoader/wadouri/getEncapsulatedImageFrame.ts
12) externalModules.ts > imageLoader/registerLoaders.ts > imageLoader/wadouri/index.ts > imageLoader/wadouri/metaData/index.ts > imageLoader/wadouri/metaData/metaDataProvider.ts
  • adapters looks fine
  • nifti looks fine

Do you have any one that can work on these?

sedghi avatar Sep 28 '23 02:09 sedghi

hummm i see the problem, for instance the xrfRequest should better event the initiation and the result of the request to a listener taht will have cornerstone reference rather then trying to get it's own cornerstone reference which create a circular dependency... The refactoring is going to be not that simple, for now celian is in school for 6w, will try to see.

salimkanoun avatar Sep 28 '23 08:09 salimkanoun

In any case, this is a great first step to find out what is wrong, though i'm not sure why results of dpdm and madge are different

sedghi avatar Sep 28 '23 11:09 sedghi

For the dicomimageloader ones there certainly need to be breaking changes, so can we merge this into beta?

sedghi avatar Sep 29 '23 01:09 sedghi

There are only 4 circ deps left report by madge

sedghi avatar Sep 29 '23 01:09 sedghi

Great !

Yeah I think you can merge this, it already reduce the problem and also prevent other commits to add more circular deps

When Célian comes back (November) he can look at the remaining circular deps

salimkanoun avatar Oct 03 '23 15:10 salimkanoun

@salimkanoun I want to change this base to beta, since it is a braking change, we ship it in 2.0

sedghi avatar Oct 16 '23 18:10 sedghi

Sounds good

salimkanoun avatar Oct 16 '23 18:10 salimkanoun

I'll close this and do it myself in the beta since it is outdated, thanks

sedghi avatar Aug 21 '24 14:08 sedghi