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

begin TS conversion #71

Closed
wants to merge 3 commits into from
Closed

Conversation

FezVrasta
Copy link
Contributor

@FezVrasta FezVrasta commented Jun 30, 2022

This is the first step in order to address #67

Let's convert the codebase to TypeScript, this way it will be much easier to migrate it from class components to hooks later, fixing #67.

I'm not sure I will have enough bandwidth to allocate to this effort so we may want to have you Kitware folks work on it if possible, we can sync offline.

/**
* set of property values for vtkClass
*/
state?: object;
Copy link
Contributor Author

@FezVrasta FezVrasta Jun 30, 2022

Choose a reason for hiding this comment

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

I used a codemod to convert prop types to TS types, they will need some improvements.

@nyoung697
Copy link

I support this endeavour, and may have some time to contribute.

/**
* Properties to set to the mapper
*/
mapper?: object;
Copy link

@nyoung697 nyoung697 Jul 1, 2022

Choose a reason for hiding this comment

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

Do these objects map directly to classes like vtkMapper, vtkVolume, vtkProperty, etc...? Could the object type eventually be converted to the types on those classes?

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'm not sure, they were defined just as Object on prop-types 😔

Copy link
Collaborator

Choose a reason for hiding this comment

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

These are objects that represent the settable properties for the mapper/property/etc. We can feasibly use the initial values types for the object constructors here. For instance, here is the vtkMapper initial values interface.

@floryst floryst mentioned this pull request Jul 18, 2022
@floryst
Copy link
Collaborator

floryst commented Sep 21, 2022

After some evaluation, this will additionally require a significant chunk of typing for certain classes in vtk.js (i.e. manipulators). This is a good thing, but will incur further overhead when making this migration.

@FezVrasta FezVrasta closed this Feb 21, 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 this pull request may close these issues.

3 participants