d3-geo SVGProps error: Type 'string | null' is not assignable to type 'string | undefined'.

[quantuminformation]

@Andy-MS I'm not sure if you are the correct person to ping but here goes.

I am getting a type error with geoPath() and Reacts <path element

Could you tell me if d=null will really work at runtime? The type definition seems to expect only string | undefined but not null.
d3-geo authors are @Ledragon @tomwanzek @gustavderdrache @borisyankov

[gustavderdrache]

I experimented with this in Codepen just now and at it doesn't seem to cause an issue with React 16.

After doing a quick search in the react-dom sources, it appears that it does a == null comparison for when to remove an attribute, which would mean that both null and undefined are acceptable. If that's the case, then this is a bug in the React typings rather than the d3-geo typings.

You could make a PR there then.

[tomwanzek]

Thanks @gustavderdrache for looking into the matter already.

A couple of comments for clarification: the d3-geo definitions are not yet "officially" validated for strictNullChecks.

So, @Ledragon or @gustavderdrache if either of you would love to volunteer for the role of PR creator or reviewer to complete the validation, I am volunteering to take the complementary role.

@quantuminformation Specifically to geoPath().projection(projection)(d) however: d3-geo follows the same pattern as all other path-generators in D3:

• if the rendering context is a canvas, it renders to such context and the path generator invocation has no return value (void/undefined)
• if the rendering context is an svg element, it returns string | null reflecting the svg-path string. It returns null if the internally streamed path has a length of 0 See here.

So despite the lack of formal validation for strictNullChecks, as far as the geoPath generator invocation is concerned, the current definitions already reflect the correct behaviour.

[Ledragon]

If no one beats me to it, I will try and tackle this (some day, without date commitment, but hopefully soon :-) ). I did not yet check how strictNullChecks works, so that's an opportunity

[tomwanzek]

@Ledragon Thx. [insert non-existing emoji here: tapping foot while looking impatiently at wrist watch] 😉
Take your time. Ping me when you are ready to start, and I can give you some pointers on how I went about the strictNullCheck validations for the D3 modules already done as per #11365 . Cheers.

[Ledragon]

Okay, so I took a first look at this... Turning the flag to true in tsconfig.json fails the tests... which is good I guess. I got the general idea of strictNullChecks. Now, the real question is: how to detect the places where null and/or undefined should be allowed?

Did you go over the code on every place (see you for Christmas), or just relied on the docs? Or a mix, just checking the code when you had doubts?

[tomwanzek]

@Ledragon My apologies, I did write an answer to your questions on the same day you posted your question. Turns out, I must have forgotten to hit the comment button before moving on. Mea culpa 😞

So back to it. The answer is: a mix.

I tend to start with the API documentation. Every so often, I would hit a method/function, where I get this funny feeling in my stomach that is not easily explained away as appendicitis. 🤔 Then I would look at the source code to check, if there are possible return values of null or void/undefined. The same for possible null values as arguments.

There are some patterns that tend to hold across D3 modules, e.g.:

• for the return type of path generators, which can render to canvas and svg context
• some setters accept null as an argument to remove and event listener or reset a setting to its default value.

Hope this helps for starters.

[Ledragon]

@tomwanzek No worry, this gave me a good excuse to do nothing :D
More seriously, time is tight atm; I will try my best to come back to this ASAP. Thanks for the guidance, this will for sure help me getting started

[tomwanzek]

@Ledragon @gustavderdrache On a more urgent note, the geojson PR #21590 just got merged. It removed the CoordinateReferenceSystem interface, which now breaks the d3-geo definitions. Not sure why the Travis CI tests did not let the proposed changes fail given the dependency of the d3-geo definitions. We'll need to address that asap.

[Ledragon]

So, I came back to this... It seems that simply adding null to GeoGeometryObjects union type solved a lot of the issue. For the rest, I simply added checks on existence of methods before calling them.

One question remains on my mind though: I did not yet check where undefined could be used in addition to null. But maybe the way overloads of methods are written makes it so that this is not required?

You can check what I did here; if that seems fine to you, I can submit a PR.

[tomwanzek]

@Ledragon I just came back online, will follow-up at my earliest.

[Ledragon]

Sure, no rush, take all the time you need

[tomwanzek]

@Ledragon Shouldn't have said no rush 😉 I am creating a little work package for myself right now. There are a couple of new minor version releases out related to D3. I will open issue and combine addressing them with looking at the technical debt I owe you on d3-geo. Will cc you as appropriate.

[Ledragon]

Hahaha let's put the blame on Christmas holidays. Nobody is complaining this is not done, so I guess there really is no rush