Skip to content

Conversation

drewhoener
Copy link
Contributor

@drewhoener drewhoener commented Aug 8, 2025

Public API Changes
None

Description

Overview

This PR migrates the classes in the math module to Typescript. As I mentioned in #904, this PR is largely trivial and just requires adding the types described in the JSDoc.

Contents

  • Adds an interface I<Type> to each so math modules can reference each other without introducing circular imports.
  • Use the same interfaces to provide constructor options
  • Adds some utility types for null and undefined

Depends on #905

GitHub doesn't have "dependent PRs" so all previous commits are included here. Might not be worth reviewing until dependencies are merged

@drewhoener drewhoener force-pushed the feature/typescript-math-module-refactor branch 2 times, most recently from 23c23d9 to 7d14ba1 Compare August 8, 2025 06:47
@drewhoener drewhoener changed the title Convert math module to TypeScript Migrate math module to TypeScript Aug 8, 2025
@drewhoener drewhoener force-pushed the feature/typescript-math-module-refactor branch from 7d14ba1 to 27dc057 Compare August 8, 2025 07:31
Copy link
Contributor

@EzraBrooks EzraBrooks left a comment

Choose a reason for hiding this comment

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

GitHub tip: if you made this PR be based on your prior PR, it would deduplicate the diff - and when that PR is merged it would automatically retarget this one to the new base branch.

@drewhoener
Copy link
Contributor Author

GitHub tip: if you made this PR be based on your prior PR, it would deduplicate the diff - and when that PR is merged it would automatically retarget this one to the new base branch.

Yeah, the 3 PRs I made are in a direct chain with each other, the branches continue from the previous one. I just mean github still shows all the commits until the previous one is merged. You can absolutely start reviewing at the first relevant commit though

@drewhoener drewhoener force-pushed the feature/typescript-math-module-refactor branch 2 times, most recently from 1bcb187 to 29a19d0 Compare August 8, 2025 18:01
@drewhoener
Copy link
Contributor Author

@EzraBrooks this one should also be ready

Copy link
Contributor

@EzraBrooks EzraBrooks left a comment

Choose a reason for hiding this comment

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

In modern typescript do we actually need the interfaces here or can we just import the class type as a type? I thought we could import type MyClass now 🤔

@drewhoener
Copy link
Contributor Author

drewhoener commented Aug 8, 2025

In modern typescript do we actually need the interfaces here or can we just import the class type as a type? I thought we could import type MyClass now 🤔

We can, the constructors define object types for their options though, so it made sense to make a type for it to also inherit. Either that or I'd have to define a type anyway for the constructor since I'm trying not to change any API semantics.
It also helps with DX for methods like Quaternion.multiply, if it accepts the interface type you don't have to be so verbose by making a new class instance.

const q: Quaternion = new Quaternion({
  x: 1,
  y: 1,
  z: 1,
  w: 1
});

// Without the interface type
q.multiply(new Quaternion({
  x: 1,
  y: 1,
  z: 1,
  w: 1
}));

// With type interface
q.multiply({
  x: 1,
  y: 1,
  z: 1,
  w: 1
});

You can still pass in a Quaternion instance, but it provides more flexibility

This commit will have lint/compile errors but is necessary for git continuity. Without it, git will think the files have been deleted and recreated because of the diff.

Signed-off-by: Drew Hoener <[email protected]>
…ce to represent structure and type methods.

Signed-off-by: Drew Hoener <[email protected]>
…nterface to represent structure and type methods.

Signed-off-by: Drew Hoener <[email protected]>
…erface to represent structure and type methods.

Signed-off-by: Drew Hoener <[email protected]>
…represent structure and type methods.

Signed-off-by: Drew Hoener <[email protected]>
@drewhoener drewhoener force-pushed the feature/typescript-math-module-refactor branch from 29a19d0 to 07c0d12 Compare August 8, 2025 18:23
Copy link
Contributor

@EzraBrooks EzraBrooks left a comment

Choose a reason for hiding this comment

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

Makes sense to me. I guess I'm just out of the object-oriented mindset 😆

@EzraBrooks EzraBrooks merged commit f594bc2 into RobotWebTools:develop Aug 8, 2025
2 checks passed
@drewhoener drewhoener deleted the feature/typescript-math-module-refactor branch August 8, 2025 19:53
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.

2 participants