-
Notifications
You must be signed in to change notification settings - Fork 320
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
fix(types): type fixes #1638
fix(types): type fixes #1638
Conversation
Prevent invalid internal type import in the generated .d.ts types file
Prevent internal import in generated .d.ts types
Run & review this pull request in StackBlitz Codeflow. |
✅ Deploy Preview for cornerstone-3d-docs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
} from '@cornerstonejs/core'; | ||
|
||
function extractWindowLevelRegionToolData(viewport) { | ||
function extractWindowLevelRegionToolData( | ||
viewport: VolumeViewport | StackViewport |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be a type, IVolumeViewport | IStackViewport i guess
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes it can be. I thought the stricter type of VolumeViewport | StackViewport
was more appropriate because the function itself throws an error if viewport
is not an instance of VolumeViewport
or StackViewport
. If the argument type accepted IVolumeViewport | IStackViewport
it would be theoretically possible to pass something else, causing an error to be thrown. Hence the stricter type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, just checking if you have a preference between the stricter viewport: VolumeViewport | StackViewport
type or viewport: IVolumeViewport | IStackViewport
. I can update the PR to suit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I remember a lot of issues with using classes as types with API Extractor in the past. It was not smart enough to use types for them, and I think the rest of the app uses types, which, in fact, are the same. If you look inside IVolumeViewport, it says:
import type { VolumeViewport } from '../RenderingEngine';
type IVolumeViewport = VolumeViewport;
export type { IVolumeViewport as default };
So I think I prefer IVolumeViewport.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No problems. Updated.
James
Use exported IVolumeViewport and IStackViewport
Context
The release includes imports to internal files in the generated typescript types. This means there are typescript errors if one of the affected packages is imported into other project, the typescript compilation fails.
For example, the compiled Typescript file
cornerstone3D/packages/tools/dist/esm/stateManagement/segmentation/helpers/getSegmentationActor.d.ts
is
which includes
export declare function getLabelmapActorEntry(viewportId: string, segmentationId: string): import("packages/core/dist/esm/types").ActorEntry;
andexport declare function getSurfaceActorEntry(viewportId: string, segmentationId: string, segmentIndex?: number | string): import("packages/core/dist/esm/types").ActorEntry;
These imports are invalid in the distributed module.
To verify:
yarn build
grep -r 'esm/' packages/**/*.d.ts
Changes & Results
In order to prevent the incorrect generate type
Before
After
This patch fixes
@cornerstonejs/tools
.However incorrect imports remain in
@cornerstonejs/dicomImageLoader
. I wasn't able to determine the correct types as some appear not to be publicly exposed currently.Testing
yarn build
grep -r 'esm/' packages/tools/**/*.d.ts
will find no incorrect imports in*.d.ts
filesChecklist
PR
semantic-release format and guidelines.
Code
My code has been well-documented (function documentation, inline comments,
etc.)
I have run the
yarn build:update-api
to update the API documentation, and havecommitted the changes to this PR. (Read more here https://www.cornerstonejs.org/docs/contribute/update-api)
Public Documentation Updates
additions or removals.
Tested Environment