Leaflet circle radius should not accept undefined radius - #69221

@Crakzzy Thank you for submitting this PR! I see this is your first time submitting to DefinitelyTyped 👋 — I'm the local bot who will help you through the process of getting things through.

This is a live comment which I will keep updated.

2 packages in this PR

• iitc — on npm, on unpkg
• leaflet — on npm, on unpkg
  • owner-approval: @henrythasler, @someonewithpc

Code Reviews

This PR can be merged once it's reviewed.

You can test the changes of this PR in the Playground.

Status

• ✅ No merge conflicts
• ✅ Continuous integration tests have passed
• ✅ Type definition owners or DT maintainers needs to approve changes which affect more than one package

All of the items on the list are green. To merge, you need to post a comment including the string "Ready to merge" to bring in your changes.

{
  "type": "info",
  "now": "-",
  "pr_number": 69221,
  "author": "Crakzzy",
  "headCommitOid": "b81b78f4d496bf8a8442eb568d6eece620d004d2",
  "mergeBaseOid": "41aa59f8e323a93b98ecea69fcf67a7a0a2cfa67",
  "lastPushDate": "2024-03-31T16:26:05.000Z",
  "lastActivityDate": "2024-04-03T16:52:53.000Z",
  "maintainerBlessed": "Waiting for Code Reviews",
  "mergeOfferDate": "2024-04-03T16:38:19.000Z",
  "mergeRequestDate": "2024-04-03T16:52:53.000Z",
  "mergeRequestUser": "Crakzzy",
  "hasMergeConflict": false,
  "isFirstContribution": true,
  "tooManyFiles": false,
  "hugeChange": false,
  "popularityLevel": "Popular",
  "pkgInfo": [
    {
      "name": "iitc",
      "kind": "edit",
      "files": [
        {
          "path": "types/iitc/core/iitctypes.d.ts",
          "kind": "definition"
        }
      ],
      "owners": [
        "McBen"
      ],
      "addedOwners": [],
      "deletedOwners": [],
      "popularityLevel": "Well-liked by everyone"
    },
    {
      "name": "leaflet",
      "kind": "edit",
      "files": [
        {
          "path": "types/leaflet/index.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/leaflet/leaflet-tests.ts",
          "kind": "test"
        }
      ],
      "owners": [
        "alejo90",
        "atd-schubert",
        "mcauer",
        "ronikar",
        "life777",
        "henrythasler",
        "captain-igloo",
        "someonewithpc"
      ],
      "addedOwners": [],
      "deletedOwners": [],
      "popularityLevel": "Popular"
    }
  ],
  "reviews": [
    {
      "type": "approved",
      "reviewer": "henrythasler",
      "date": "2024-04-03T16:44:46.000Z",
      "isMaintainer": false
    },
    {
      "type": "approved",
      "reviewer": "someonewithpc",
      "date": "2024-04-03T16:04:35.000Z",
      "isMaintainer": false
    }
  ],
  "mainBotCommentID": 2028814457,
  "ciResult": "pass"
}

🔔 @McBen @alejo90 @atd-schubert @mcauer @ronikar @life777 @henrythasler @captain-igloo @someonewithpc — please review this PR in the next few days. Be sure to explicitly select Approve or Request Changes in the GitHub UI so I know what's going on.

According to https://leafletjs.com/reference.html#circle-option, CircleOptions.radius must always be a number.

I confirmed in the code, and you're right. @Crakzzy please add a test to prevent a possible future regression

According to https://leafletjs.com/reference.html#circle-option, CircleOptions.radius must always be a number.

I confirmed in the code, and you're right. @Crakzzy please add a test to prevent a possible future regression

Okay, right on it 👍

@Crakzzy The CI build failed! Please review the logs for more information.

Once you've pushed the fixes, the build will automatically re-run. Thanks!

Note: builds which are failing do not end up on the list of PRs for the DT maintainers to review.

@Crakzzy The CI build failed! Please review the logs for more information.

Once you've pushed the fixes, the build will automatically re-run. Thanks!

Note: builds which are failing do not end up on the list of PRs for the DT maintainers to review.

Formatting

The following files are not formatted:

1. types/leaflet/leaflet-tests.ts

Consider running pnpm dprint fmt on these files to make review easier.

Generated by 🚫 dangerJS against b81b78f

@henrythasler Thank you for reviewing this PR! The author has pushed new commits since your last review. Could you take another look and submit a fresh review?

@Crakzzy One or more reviewers has requested changes. Please address their comments. I'll be back once they sign off or you've pushed new commits. Thank you!

@Crakzzy The CI build failed! Please review the logs for more information.

Once you've pushed the fixes, the build will automatically re-run. Thanks!

Note: builds which are failing do not end up on the list of PRs for the DT maintainers to review.

The test isn't meaningful, as it asserts that 10 is a number, rather than that the options are meaningful. To this end, I made #69238, after which it will be possible to test like:

let circle = new L.Circle(latLng, 10);
circle = new L.Circle(latLng, { radius: 10 });
// @ts-expect-error
circle = new L.Circle(latLng, { radius: undefined });
// @ts-expect-error
circle = new L.Circle(latLng, { radius: null });
// @ts-expect-error
circle = new L.Circle(latLng, { radius: NaN });
// @ts-expect-error
circle = new L.Circle(latLng, { radius: '10' });
// @ts-expect-error
circle = new L.Circle(latLng, { });
// @ts-expect-error
circle = new L.Circle(latLng);

Additionally, CircleOptions should be written in terms of CircleMarkerOptions, and that should be changed to require the radius, and the factory functions should mark the options as required

For example, the following commit could replace the commits in this PR

commit eb14ccb4f403cf09570f586d8406e542cf794da5
Author: Hugo Sales
Date:   Mon Apr 1 21:04:57 2024 +0000

    [leaflet] Disallow `undefined` in `CircleOptions.radius`

diff --git a/types/leaflet/index.d.ts b/types/leaflet/index.d.ts
index c8e90ef882..3b1ccaaeb0 100644
--- a/types/leaflet/index.d.ts
+++ b/types/leaflet/index.d.ts
@@ -2082,11 +2082,11 @@ export class Rectangle<P = any> extends Polygon<P> {
 export function rectangle(latLngBounds: LatLngBoundsExpression, options?: PolylineOptions): Rectangle;
 
 export interface CircleMarkerOptions extends PathOptions {
-    radius?: number | undefined;
+    radius: number;
 }
 
 export class CircleMarker<P = any> extends Path {
-    constructor(latlng: LatLngExpression, options?: CircleMarkerOptions);
+    constructor(latlng: LatLngExpression, options: CircleMarkerOptions);
     toGeoJSON(precision?: number | false): geojson.Feature<geojson.Point, P>;
     setLatLng(latLng: LatLngExpression): this;
     getLatLng(): LatLng;
@@ -2100,12 +2100,10 @@ export class CircleMarker<P = any> extends Path {
 
 export function circleMarker(latlng: LatLngExpression, options?: CircleMarkerOptions): CircleMarker;
 
-export interface CircleOptions extends PathOptions {
-    radius: number | undefined;
-}
+export type CircleOptions = CircleMarkerOptions;
 
 export class Circle<P = any> extends CircleMarker<P> {
-    constructor(latlng: LatLngExpression, options?: CircleOptions);
+    constructor(latlng: LatLngExpression, options: CircleOptions);
     constructor(latlng: LatLngExpression, radius: number, options?: CircleOptions); // deprecated!
     toGeoJSON(precision?: number | false): any;
     getBounds(): LatLngBounds;
@@ -2114,8 +2112,8 @@ export class Circle<P = any> extends CircleMarker<P> {
     setStyle(style: PathOptions): this;
 }
 
-export function circle(latlng: LatLngExpression, options?: CircleMarkerOptions): Circle;
-export function circle(latlng: LatLngExpression, radius: number, options?: CircleMarkerOptions): Circle; // deprecated!
+export function circle(latlng: LatLngExpression, options: CircleOptions): Circle;
+export function circle(latlng: LatLngExpression, radius: number, options?: CircleOptions): Circle; // deprecated!
 
 export interface RendererOptions extends LayerOptions {
     padding?: number | undefined;
diff --git a/types/leaflet/leaflet-tests.ts b/types/leaflet/leaflet-tests.ts
index 4a7254c3a9..e27cff7a0d 100644
--- a/types/leaflet/leaflet-tests.ts
+++ b/types/leaflet/leaflet-tests.ts
@@ -990,3 +990,17 @@ L.GeoJSON.geometryToLayer(
         },
     },
 );
+
+let circle = new L.Circle(latLng, 10);
+circle = new L.Circle(latLng, { radius: 10 });
+// @ts-expect-error
+circle = new L.Circle(latLng, { radius: undefined });
+// @ts-expect-error
+circle = new L.Circle(latLng, { radius: null });
+// @ts-expect-error
+circle = new L.Circle(latLng, { radius: '10' });
+// @ts-expect-error
+circle = new L.Circle(latLng, { });
+// @ts-expect-error
+circle = new L.Circle(latLng);

@someonewithpc
I've changed everything you provided, but the tests seem to fail on some Portal type. I've never contributed or written tests for anything, so I apologise for making rookie mistakes. I just wanted to help out. Could you please explain this issue and how to fix it? Thank you so much. I'd want to make this PR to the end and any help is useful for me.

I've never contributed or written tests for anything, so I apologise for making rookie mistakes. I just wanted to help out. Could you please explain this issue and how to fix it? Thank you so much. I'd want to make this PR to the end and any help is useful for me.

I'm sorry if I came across as mean, earlier. There's no need to apologize, you didn't do anything wrong. Making mistakes is normal, and everyone has to start somewhere. I'd recommend you to run the tests locally, next time, that way no one knows if you make any mistake ;) I'd also encourage you learn git rebase -i (and git reflog) to learn how to squash multiple commits into one. hmu by email if you need help

I've changed everything you provided, but the tests seem to fail on some Portal type.

To fix that error, you can run the following command (I recommend taking the time to understand what each piece of the command does) to apply a patch to iitc. We'll have to see if the maintainers of that package agree with this change, as I'm not familiar with it

$ git -C $(git rev-parse --show-toplevel) apply <<EOF
diff --git a/types/iitc/core/iitctypes.d.ts b/types/iitc/core/iitctypes.d.ts
index 207cd1bf81..6dab822329 100644
--- a/types/iitc/core/iitctypes.d.ts
+++ b/types/iitc/core/iitctypes.d.ts
@@ -12,7 +12,7 @@ export namespace IITC {
         options: PortalOptions;
     }
 
-    interface PortalOptions extends L.PathOptions {
+    interface PortalOptions extends L.CircleMarkerOptions {
         guid: PortalGUID;
         ent: any;
         level: number;
EOF

I've never contributed or written tests for anything, so I apologise for making rookie mistakes. I just wanted to help out. Could you please explain this issue and how to fix it? Thank you so much. I'd want to make this PR to the end and any help is useful for me.

I'm sorry if I came across as mean, earlier. There's no need to apologize, you didn't do anything wrong. Making mistakes is normal, and everyone has to start somewhere. I'd recommend you to run the tests locally, next time, that way no one knows if you make any mistake ;) I'd also encourage you learn git rebase -i (and git reflog) to learn how to squash multiple commits into one. hmu by email if you need help

I've changed everything you provided, but the tests seem to fail on some Portal type.

To fix that error, you can run the following command (I recommend taking the time to understand what each piece of the command does) to apply a patch to iitc. We'll have to see if the maintainers of that package agree with this change, as I'm not familiar with it

$ git -C $(git rev-parse --show-toplevel) apply <<EOF
diff --git a/types/iitc/core/iitctypes.d.ts b/types/iitc/core/iitctypes.d.ts
index 207cd1bf81..6dab822329 100644
--- a/types/iitc/core/iitctypes.d.ts
+++ b/types/iitc/core/iitctypes.d.ts
@@ -12,7 +12,7 @@ export namespace IITC {
         options: PortalOptions;
     }
 
-    interface PortalOptions extends L.PathOptions {
+    interface PortalOptions extends L.CircleMarkerOptions {
         guid: PortalGUID;
         ent: any;
         level: number;
EOF

No, you weren't mean, don't worry. The change you provided helped to fix the error. Although that error is fixed, there are still 2 more. In the tests you provided earlier, these two still don't throw an error:

// @ts-expect-error
circle = new L.Circle(latLng, {radius: undefined});
// @ts-expect-error
circle = new L.Circle(latLng, {radius: null});

And the remaining ones do. I'm not sure why only null and undefined do this. Am I missing something 😄? I know how to use the interactive rebase for squashing the commit. I will do it at the end to clean out the PR. Again, thanks for your response, and could you please let me know why is this happening?

I'm not sure why only null and undefined do this

With the strictNullChecks compiler option set to false as it is currently, all types T are actually T | undefined | null, which is why I said this should wait for #69238 Though, it could go before, if we remove those two test lines from this and add it to the other, which is probably better, in this case, so if you could, please remove those two tests and clean it up and I'll merge it

I'm not sure why only null and undefined do this

With the strictNullChecks compiler option set to false as it is currently, all types T are actually T | undefined | null, which is why I said this should wait for #69238 Though, it could go before, if we remove those two test lines from this and add it to the other, which is probably better, in this case, so if you could, please remove those two tests and clean it up and I'll merge it

If you don't mind, I will do it tomorrow morning. And thanks for the explanation 👍

@someonewithpc, @henrythasler Thank you for reviewing this PR! The author has pushed new commits since your last review. Could you take another look and submit a fresh review?

@someonewithpc
Now it should be cleaned up and ready to merge 👍

@McBen This also changes iitc, could you review this?

Good job adding the tests.

Thanks, but @someonewithpc really helped me. I've never written any tests so I've learned something new 👍.

Ready to merge

..okay, I see it's already done.
but let me add a remark here:

IITC uses old Leaflet v0.77. I'm not sure if old leaflet types already included the CircleMarkerOption.

RN I don't have the time to move the old IITC project (no longer developed) types to the successor project IITC-CE.

IITC uses old Leaflet v0.77

This package provides types for v0.7, though I don't know and don't want to bother checking whether that preceeded your package

I'm not sure if old leaflet types already included the CircleMarkerOption.

It seems it didn't, which would explain why you wrote it like that

@Crakzzy
There is an issue with the setStyle method, which requires radius now. It should be still optional in setStyle.

@Crakzzy There is an issue with the setStyle method, which requires radius now. It should be still optional in setStyle.

@lmachens
I see, my suggestion is to leave the CircleMarkerOptions as they are now with the radius mandatory and create a new interface specifically for the setStyle option which will be the same as the CircleMarkerOptions with the difference of radius being optional. Something like this:

export interface CircleMarkerStyleOptions extends PathOptions {
    radius?: number | undefined;
}

And then:

- setStyle(options: CircleMarkerOptions): this;
+ setStyle(options: CircleMarkerStyleOptions): this;

Or if you have a better idea, let me know 😃.