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

Consider re-exporting @types/geojson types #2769

Closed
ijxy opened this issue Dec 14, 2024 · 7 comments
Closed

Consider re-exporting @types/geojson types #2769

ijxy opened this issue Dec 14, 2024 · 7 comments

Comments

@ijxy
Copy link

ijxy commented Dec 14, 2024

@types/geojson is an explicit dependency in various packages, but, despite that the types, are not exported.

If I can use @turf/helpers to create a thing, I should be able to rely on @turf/helpers to give me the type of the thing I create.

Currently, the typical consumer journey probably goes like this:

  1. try and import the types, waste some time trying various import statements from various @turf/* packages
  2. give up and google it
  3. read that they are supposed to install and import the types (that are already there!) from a separate package (!)

For the love of code, please just make it easy to do the "RightThing". Re-export the types!

import { toPoint, Point } from "@turf/helpers"; // Obviously the "RightThing"
import { toPoint } from "@turf/helpers"; 
import type { Point } from "geojson";  // same version? is this right? Why don't we have the "geojson" package installed as well? Oh that is unrelated and these types are from "@types/geojson" under the hood?! And double the import statements, yay!
@smallsaucepan
Copy link
Member

Thanks for your comments @ijxy. Have clarified in the README that @types/geojson needs to be installed separately:

https://github.com/Turfjs/turf/blob/master/README.md#typescript

@ijxy
Copy link
Author

ijxy commented Dec 14, 2024

Thanks for your comments @ijxy. Have clarified in the README that @types/geojson needs to be installed separately:

https://github.com/Turfjs/turf/blob/master/README.md#typescript

It is still a terrible DX though, which is why I am asking you to consider changing your stance. It costs Turf nothing to re-export the types, but it costs all users time and energy figuring out that the types aren't re-exported (people typically consult a README after they get stuck, not before).

Again, it's also a matter of encapsulation, as in the example I provided above.

I have seen this many times in other corners of the web, if you do not export the types then you're essentially encouraging code such as the following:

import { toPoint } from "@turf/helpers";

export type Point = ReturnType<typeof toPoint>;

In a large codebase, it is unreasonable to expect every dev to come and read the README for every package. So instead, you strive to make it easy to understand what is happening.

This is obvious to all:

import { toPoint, Point } from "@turf/helpers";

This is not:

import { toPoint } from "@turf/helpers";
import type { Point } from "totally-different-package-that-isn't-in-our-package.json-file"

The fact that @types/geojson has nothing at all to do with the geojson package makes it all the more confusing, but that is another story.

Appreciate the quick response, by the way, and respect your decision either way. Just wanted to make a strong case for it as I believe it is net beneficial to all Turf users by reducing mental overhead.

@smallsaucepan
Copy link
Member

Can certainly see it from your point of view. The situation is weird in a lot of ways. It being a "spec only" typedef package is unusual for a start.

Ultimately, the types need to be imported from somewhere. To avoid adding unneeded coupling and possible version mismatches, it makes the most sense for Turf to refer to an external, definitive resource.

From there happy to take feedback on ways we can make it less of a learning curve for devs e.g. documentation.

The fact that @types/geojson has nothing at all to do with the geojson package makes it all the more confusing, but that is another story.

I know right? It's bonkers.

Fwiw, Turf used to re-export the types and it was a pretty popular decision to remove that (#1658). Not saying you're wrong to have a different opinion. Just that we have to make a choice sometimes based on a range of considerations, and do our best to make up the gap in other ways.

Appreciate your passion for the topic and for making the argument.

@ijxy
Copy link
Author

ijxy commented Dec 15, 2024

The linked issue, as I read it, is about replacing custom type definitions with @types/geojson "spec" types—that is something I totally agree with! Spec types is win for everyone in this case.

To avoid adding unneeded coupling and possible version mismatches, it makes the most sense for Turf to refer to an external, definitive resource.

I agree!

The Turf APIs are coupled to these types–they return instances of those types in many cases. Exporting the types would not increase the coupling at all, it would just make it easier to use the types of the objects that Turf allows you to create.

Edit: Also, I note that even you were caught out by the import being from geojson instead of @types/geojson in your initial README changes–and I missed your mistake when I looked at the README changes, too! I think that is proof enough that it is sufficiently confusing to warrant a "happy path" option of just importing the types from Turf.

@ir-fuel
Copy link

ir-fuel commented Jan 22, 2025

Thanks for your comments @ijxy. Have clarified in the README that @types/geojson needs to be installed separately:

https://github.com/Turfjs/turf/blob/master/README.md#typescript

Has something changed? There is no mention of Typescript in the readme.

@smallsaucepan
Copy link
Member

Hi @ir-fuel. The readme has since been refactored. A more up to date link would be https://turfjs.org/docs/getting-started#typescript

@ijxy
Copy link
Author

ijxy commented Jan 23, 2025

@smallsaucepan I still don't understand the reluctance to re-export the types.

Re-exporting the types would cost literally nothing in terms of maintenance (because you're not defining the types, just exporting them) or bundle size (types don't exist after transpilation). It would take a few minutes to set up the exports, but this time would be recouped many times over by consumers, since...

Re-exporting the types would make consuming these awesome packages easier (no additional dependency, no potential version issues, no need to understand the mess that is "geojson" and "@types/geojson").

When you need documentation to explain to devs how to import/access the types that your entire suite of packages is built around, then you've probably got it wrong.

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

No branches or pull requests

3 participants