Skip to content
This repository has been archived by the owner on Jul 16, 2023. It is now read-only.

add null/undefined union types to definitions for strict null checking #113

Closed
wants to merge 1 commit into from
Closed

Conversation

jiemakel
Copy link

I want to use the d3 v4 definitions (thank you very much for doing them BTW) in a project that has --strictNullChecks enabled. At certain points, the d3 definitions themselves made references to nulls and undefineds that caused errors in this mode. This PR adds the required null/undefined union types to the d3 definitions that make these errors go away (but don't affect usage in any way in non strict null checking mode).

What is not in this PR are any tests to make sure that compilation under --strictNullChecks succeeds. I tried to fiddle around with the Gruntfile a bit to create such, but for some reason changing the strictNullChecks option there to true there did nothing, so in the end I just abandoned the effort altogether.

@tomwanzek
Copy link
Owner

@jiemakel Thanks for the comment and proposed PR. You are correct, that the D3 API docs are explicit in some cases about the return type for a well-defined 'failure mode', e.g. null or undefined. Unfortunately, I did not yet have the time to do a complete sweep through the source code to confirm comprehensively where methods return with | null or | undefined or even | null | undefined. I.e. for those where the API doc is not explicit.

As for the compiler option, the above is the reason, why I have so far left it as false. I did not want to give the impression, that the definitions are strictly speaking strict yet.

Before I consider merging, let me consider the broader implication of completing the strictness verification across all modules consistently.

As opposed to DefinitelyTyped, this repo uses a single compilation context, so flicking the switch would affect all modules equally. This is a bit of a historic artifact, of how this repo evolved.

I'll respond more completely shortly. Cheers.

@tomwanzek
Copy link
Owner

@jiemakel my apologies for the delay in getting back to this. With respect to this repo, I prioritized the migration to DT/types-2.0 and npm @types by implication. There were a couple of hurdles to take, so that
(a) the production version of the definitions is accessible to the masses (with correct version numbers on npm), and
(b) there is less effort in synchronizing the changes while the migration is ongoing.

That being said, for all intents and purposes the migration is complete for all the standard bundle D3 modules. So, my preference would be to move this open item over to DefinitelyTyped. The current main tracking issue on DT is 9936 for your reference.

So as part of the migration, I will move this issue over with the considered changes. The benefit is also, that we could work through it on a module-by-module basis, validate the API w.r.t to strictNullChecking, enhance it, update tests and JSDoc comments.

The latter is another one of these to-dos, best handled in DT now.

Hope this is agreeable. I will copy you when I move this item over to DT. In the meantime, I will leave this open for reference. Thx.

@tomwanzek
Copy link
Owner

As discussed, the issue has been migrated to DefinitelyTyped as a work item under issue 11365

@jiemakel I will leave this PR open, until your specific proposed changes could be migrated into PRs on DefinitelyTyped against the types-2,0 branch. Those should be the source of truth from now on.

@tomwanzek
Copy link
Owner

See DT PR 12858 for submission of d3-selection, d3-transition, d3-selection-multi and d3-force as first batch. At long last... 😄

The general tracking issue remains: DT Issue 11365

I will close this PR now, given the actioned follow-up after catering to some other priorities in between. Cheers, T

@tomwanzek tomwanzek closed this Nov 22, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants