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

refactor: migrate Coordinates and Ellipsoid to typescript #2437

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

Desplandis
Copy link
Contributor

@Desplandis Desplandis commented Oct 11, 2024

Description

This PR includes:

  • Migration of Coordinates from javascript to typescript
  • Migration of Ellipsoid from javascript to typescript
  • Add missing types
  • Code formatting
  • TSDoc compliant documentation

Proposed breaking type changes:

  • For simplification and alignment with Three.Vector3, I propose that the constructor of Coordinates only accepts numbers as parameters. We could set from array, coordinates-like objects from corresponding methods setFromArray() or setFromVector3. Note that for js users, we would not for now break compatibility (maybe add a depreciation notice).

Motivation and Context

As described in proposal #2396, we aim to gradually migrate our entire codebase (with the exception of deprecated modules) from javascript to typescript. We choose to start with modules with no-dependency and move up in the dependency tree.

This PR is the second step of Migrate geographic modules, migrating the both co-dependent modules Coordinates and Ellipsoid. This PR is a follow-up of #2436.

@Desplandis Desplandis changed the title Ts migration/coords refactor: migrate Coordinates and Ellipsoid to typescript Oct 11, 2024
@Desplandis Desplandis force-pushed the ts-migration/coords branch 4 times, most recently from 8406a04 to 98fe688 Compare October 17, 2024 19:30
@Desplandis Desplandis marked this pull request as ready for review October 18, 2024 07:15
@jailln jailln self-assigned this Nov 18, 2024
@jailln jailln requested review from AnthonyGlt and removed request for jailln November 26, 2024 16:53
Copy link
Contributor

@AnthonyGlt AnthonyGlt left a comment

Choose a reason for hiding this comment

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

Thanks for the MR, well done :D

Should we not specified the return type of the different function of these new TS files ?
For example, for the function cartesianToCartographic of Ellipsoid
In the JS doc, we were specifying the return type, it was helping while reading the code.
I get that TypeScript is capable of inferring the return type but could be nice to still write it, like you did in Coordinates.ts. WDYT ?

this._normal = new THREE.Vector3();

// eslint-disable-next-line @typescript-eslint/no-explicit-any
if ((v0 as any).length > 0) { // deepscan-disable-line
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if ((v0 as any).length > 0) { // deepscan-disable-line
if ((x as any).length > 0) { // deepscan-disable-line

?

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 would like to remove the instanciation of Coordinates with an array or a vector3 for typescript users but not break the API for javascript users until a major version bump. However, it seems the deepscan static checker can infer that x (even if cast as any) has no property length and the CI failed. This is will be removed with the next major version bump.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand why we do the condition on v0 though ? It's not given by the user , isn't ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Holy crap, I shall read more carefully, this is a suggested change, thought it was my code. xD
I indeed intended to check the value of x, will fix it!

@Desplandis
Copy link
Contributor Author

Desplandis commented Dec 18, 2024

@AnthonyGlt Return types in Ellipsoid are now explicit!

@Desplandis Desplandis force-pushed the ts-migration/coords branch 5 times, most recently from d4960ea to 7a56b5f Compare December 19, 2024 11:33
@Desplandis
Copy link
Contributor Author

@AnthonyGlt All good!

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