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

refacto: migrate Crs to typescript #2436

Open
wants to merge 8 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 Crs from javascript to typescript
  • Fix of type issues (mainly undefined case not treated)
  • Add a ProjectionLike type alias for named CRS which could be later extended as either a string or a proj4 definition object (à la OpenLayer)
  • TSDoc compliant documentation
  • Add more unit tests

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 first step of Migrate geographic modules, migrating the base module Crs. Follow-up steps includes migration of dependent modules Coordinates/Ellipsoid and Extent.

src/Core/Geographic/Crs.ts Outdated Show resolved Hide resolved
src/Core/Geographic/Crs.ts Outdated Show resolved Hide resolved
src/Core/Geographic/Crs.ts Outdated Show resolved Hide resolved
src/Core/Geographic/Crs.ts Outdated Show resolved Hide resolved
test/unit/crs.js Show resolved Hide resolved
src/Core/Geographic/Crs.ts Outdated Show resolved Hide resolved
src/Core/Geographic/Crs.ts Outdated Show resolved Hide resolved
@jailln jailln self-requested a review October 17, 2024 08:34
@Desplandis Desplandis force-pushed the ts-migration/crs branch 3 times, most recently from 06fa1e1 to 33c57e4 Compare October 17, 2024 20:08
Copy link
Contributor

@jailln jailln left a comment

Choose a reason for hiding this comment

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

Thanks :) Nice to see that we are moving forward on this migration 👍

While we're at it, I would be down for renaming some functions to make their name clearer:

  • toUnit -> getUnit
  • reasonnableEpsilon -> reasonableEpsilon
  • formatToEPSG -> formatToEpsg (to be consistent with capital letters in other methods). Or use capitals in all methods with EPSG and TMS.

I would also make private is4326.

Also, we could add a isDegreeUnit since we expose a isMetricUnit.

package.json Show resolved Hide resolved
src/Core/Geographic/Crs.ts Show resolved Hide resolved
/**
* A projection as a CRS identifier string.
*/
export type ProjectionLike = string;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we export it in the doc ? Also, the comment is not complete as it can either be a crs identifier or a wkt string describing the projection and passed directly to proj4 (in isGeocentric for instance)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, we should export it in the documentation since this is the type of the argument of some functions of the CRS module.

I think the documentation is complete, this is an identifier referencing an already defined projection (I should maybe add this to the documentation). In isGeocentric, the call to proj4.defs is only for retrieving an already defined projection as an object of type proj4.ProjectionDefinition.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moreover, I added such type alias since we'll surely want in a near future to have a ProjectionLike either as an identifier or an proj4.ProjectionDefinition.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh ok thanks, proj4.defs is quite misleading 😅

I just checked and it is not exported in the doc though, you may need to add a @typedef tag like for CameraTransformOptions? : @typedef {Object} CameraTransformOptions

Also, I'm wandering if it should appear in the Crs doc page or have its own page or be in a Types page, WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The equivalent of typedef in typescript or either an interface or a type. Instead of being in the documentation, we can write the types directly.

src/Core/Geographic/Crs.ts Outdated Show resolved Hide resolved
src/Core/Geographic/Crs.ts Outdated Show resolved Hide resolved
src/Core/Geographic/Crs.ts Outdated Show resolved Hide resolved
src/Core/Geographic/Crs.ts Outdated Show resolved Hide resolved
src/Core/Geographic/Crs.ts Outdated Show resolved Hide resolved
@Desplandis
Copy link
Contributor Author

@jailln I fixed all issues listed in your review.

I added an @internal tag for all functions that I deemed to be only used internally by itowns. This mainly includes all related epsg and tms functions, that may be removed soon since we split Extent and Tile. I only kept the TMS naming scheme for Tile to identify where Tile was used but we could use the EPSG naming scheme (removing the need for the format and test functions).

* `toUnit` -> `getUnit`

Done, shall I mark it as breaking change though?

* `formatToEPSG` -> `formatToEpsg` (to be consistent with capital letters in other methods). Or use capitals in all methods with EPSG and TMS.

I chose to kept the old name since we'll surely remove it due to the reasons above.

Also, we could add a isDegreeUnit since we expose a isMetricUnit.
I think we could add it later if the need arise.

@jailln
Copy link
Contributor

jailln commented Nov 18, 2024

Thanks for the changes :)

@jailln I fixed all issues listed in your review.

I added an @internal tag for all functions that I deemed to be only used internally by itowns. This mainly includes all related epsg and tms functions, that may be removed soon since we split Extent and Tile. I only kept the TMS naming scheme for Tile to identify where Tile was used but we could use the EPSG naming scheme (removing the need for the format and test functions).

I would be clearer to only use the EPSG naming scheme yes.

* `toUnit` -> `getUnit`

Done, shall I mark it as breaking change though?

I'm not sure it is used but we might add a breaking change to the commit message just in case.

@Desplandis
Copy link
Contributor Author

Desplandis commented Nov 21, 2024

Okay, I went a little bit further and removed the two naming schemes (which were not needed since #2412). This means that all espg/tms functions are not needed anymore, so I removed them too. However, shall I keep the espg/tms functions for backward compatibility (and deprecated them) but remove reference in the codebase?

I don't think they are used outside of the codebase IMO.

As for the documentation, ProjectionLike documentation is not generated with JSDoc but is generated with TypeDoc. I however did change our documentation engine for now, since it will introduce a lot of conflicts with #2414. Waiting for it to be merged!

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