d3 / d3

Bring data to life with SVG, Canvas and HTML. :bar_chart::chart_with_upwards_trend::tada:
https://d3js.org
ISC License
108.55k stars 22.86k forks source link

[RFC]: rewrite in typescript and restructure to monorepo #3284

Closed sandangel closed 6 years ago

sandangel commented 6 years ago

Problem:

Solution:

Roadmap:

curran commented 6 years ago

Regarding the Problem section:

sandangel commented 6 years ago

Hi @curran, I suppose that having mono repo is better because:

Besides that, the amount of typescript users using d3 is large but the percentage of users using @types/d3 is quite small because as I said, ~it completely useless.~ It does not correctly reflect the API in many usecase and I have to use any type many times for ts to compile. Re-writing in ts will help us keep typing in sync in every release, and free up 3 maintainers for contributing to d3 typescript instead of @types/d3

I believe that by rewrite in ts and move to monorepo, we can refactor and absract more reusable code which will also help reduce package size which quite large for now.

PS: I really admire your Vim skill in D3.js in Motion ^^

curran commented 6 years ago

Thanks!

Ledragon commented 6 years ago

@sandangel If you think the typings for d3 in DefinitelyTyped are out of date, feel free to submit tickets there. We try as much as we can to keep these up-to-date, which I think thay are at the moment, so I am surprised you find these useless. Maybe you can open an issue over there explaining the problems you face and how you think these should be handled

sandangel commented 6 years ago

Hi @Ledragon, Sorry I respect your work. May be I need to carefully choose a better word than useless.

I thought of firing issue about d3 typings before, but I regconize it wont solve the source of problem. You always have to track the source changes and for the long term it will cost more headache when the project evolves. So why dont we just re-write it once and we will no longer have to care about typings issue. Once again I’m so sorry about words I have used that make you feel bad. I have updated my comment properly and I hope it appropriate this time.

Ledragon commented 6 years ago

@sandangel No offence taken (I thought it might sound like it while replying, but really, not at all :-) ). Your feedback is important, as the goal is to help people using d3 with typescript. If the work that is done does not achieve that, then maybe we missed a point. I totally concur to the idea of maintaining types together with the project, and raised the question to @tomwanzek, who redirected me to a former issue, where @mbostock replied that this was not the intent at the time. But maybe it is time to raise the question again, with the current rise of typescript...

sandangel commented 6 years ago

Yeah, I saw that issue too and it was 2013, almost 5 years ago. Now we have tools like typewiz which can help us rewrite the code faster. I hope that @mbostock will accept this.

mbostock commented 6 years ago

Thank you for using D3, giving feedback and suggesting improvements. Alas, I have no plans to rewrite D3 in TypeScript, nor do I have the bandwidth to support such an effort. You are welcome to fork D3 to rewrite it and TypeScript, or to contribute to its type definitions (championed by @tomwanzek!).

I also have no plans to switch to a monorepo. D3 v4 spawned multiple repositories to allow separate owners for modules without requiring central coordination. That said, I do see some clear advantages to a monorepo, and if I could go back in time, I might have done that instead. It is very tedious to update patterns used across the repositories (see #3116 and wip d3-scripts); I would like to improve my efficiency at releasing. But the colossal effort required to switch back to a monorepo and the likely disruption that would cause on users has prevented me from taking such drastic action, so I’ve been looking for more incremental ways to improve the design of D3 modules. Happy to hear further suggestions on this topic!

tomwanzek commented 6 years ago

With respect to maintaining TS definitions within the actual D3 modules, I have always deferred to @mbostock as the principal owner. Always open to considering a migration and discussion, how we could most efficiently maintain them here without unduly impacting his release cycles.

Here is some background:

When we checked last month, the installations that use D3 + TS Definitions are about >15% inferred from npm download stats (give or take differences in popularity for individual modules that are downloaded individually).

As @Ledragon already mentioned, the D3-related TS definitions are current for the 30+ D3 modules he, @gustavderdrache and myself have supported since the refactoring @mbostock has done with D3 v4. In our experience, generally these TS definitions reflect the latest published version of a D3 module within less than a week after new version release. (Some of the cycle time is due to DefinitelyTyped merge/pub process but again generally the Microsoft folks have been very responsive to get approved PRs published to npm quickly). Since the initial release stabilization of D3 v4 and the first cut of definitions, we have seen about 10 fix requests across all the modules we support (over about 1 1/2 years). About half of them were related to relaxing argument types, which were too strict, some others were about arguments/properties which were optional, rather than mandatory.

While we are working through the definitions to fully validate them for strictNullChecks and strictFunctionTypes, they are feature complete including the extensive use of generic parameters to control type-safety on the extensive D3 API Mike has developed over the years. strictNullChecks validations are a bit more involved, as we have to go deeper into the source code in some case to establish D3 behavior. Most PR activity is related to enhancing the definitions (see tracking issue DefinitelyTyped/DefinitelyTyped#23611) or matching D3 module minor/major releases.

@sandangel Should you have identified a particular issue where you believe the TS definitions do not reflect the D3 API, please feel free to open an issue on DefinitelyTyped for the affected definition(s) making sure to @-mention the involved definitions owners. When doing so, please ensure to provide a concise issue description, as it is not clear to me why you always would have to "use any". As mentioned, we make extensive use of generics with constraints to allow use-case specific type-safety, which includes the ability to strongly type the this context of D3 methods/functions operating on bound DOM elements. So, should you have questions on how to use the generics effectively, feel free to use StackOverflow.

tomwanzek commented 6 years ago

Sorry, for added clarity, with respect to a re-write of D3 in TypeScript, while I personally would love it, I respect that Mike has written very compact code that packs a lot of features into a very small code base. In order do achieve this important goal, he used some coding patterns which exploit the dynamic nature of JS. D3 also makes extensive use of type-coercion.

So re-writing D3 in TypeScript, would not be as simple as renaming a file to .ts and adding some type annotations. It would clearly be more involved.

gustavderdrache commented 6 years ago

Speaking as the author of the current D3v3 typings (and occasional helper on v4 when @tomwanzek or @Ledragon don't beat me to it!), maintaining types for D3 can be trickier than it looks and we welcome any and all improvements. The D3 API surface is incredibly flexible, and it's taken a lot of collective brainpower to reflect this flexibility in a statically-typed world.

I've authored and maintained a number of production codebases using both the v3 and v4 @types packages and can't remember a case where I've needed to revert to an any type. I suggest opening issues in the DefinitelyTyped repo with demonstration code to see if we can't put our heads together to solve the problems you're seeing.

sandangel commented 6 years ago

Hi everyone, I respect your work and want to help improve types/D3. But I think maintaining types/d3 seperately would cost more effort in the long term camparing with rewriting it once for all. With the new version of ts2.8 which support conditional type, we could improve more type inference. but I think it would be trickier to change all the types definition follow conditional type than generate it from source.

So I suggest that we should fork this repo and start rewriting it in ts, so when @mbostock release a new version, we just have to read through his commit and change our code accordingly, generate typings and publish it as types/d3 as before (ts now support generate typings only as I remember). I think this way help maintaining types/d3 a lot easier. Optionally we could publish a d3-ts package and use webpack 4 side-effect free feature.

When @mbostock has time to look into our work, hopefully he will consider merge it into the main project.

tomwanzek commented 6 years ago

As far as I am concerned, any meaningful conversion of D3 into TypeScript sources would only be sustainable with Mike's collaboration. There will be aspects of a rewrite that will go beyond mere type annotations. I caution not to underestimate the resulting effort if one aims for a comparably solid codebase. Currently, the actual maintenance effort of the TS definitions is minimal (technical debt aside). Unless Mike has some major breaking fancy plans for D3 v6 :wink:, we are at a very stable base right now.

Forking and rewriting D3 in its entirety in TS without Mike's buy-in will be much harder to sustain on the other hand. Every fix/patch release to D3 would need to be reviewed and (likely) mapped to the TS codebase as well.

I would also caution just running after the latest minor releases of TS. As indicated, by now there is a material installed base that uses the TS definitions. We are now slowly moving the definitions to a minimum of TS 2.3. By now this version is out for ~1 year and we feel relatively comfortable that we won't break someone who should not be able to update their legacy project to 2.3 by now.

That being said, as the nature of Open Source goes, @sandangel feel free to fork.

sandangel commented 6 years ago

hi @tomwanzek , for the choice of using ts2.8, I believe that rewriting d3 would be time consuming and by the time we finished, users would have already been using ts2.8.

And typings is not the only reason why I have suggested rewrite d3. We should consider both pros and cons and I don’t see any technical cons for the long term when the project evolves. The only problem here is time and effort we have to spend when dealing with such large codebase like d3. But fortunately, d3 is splitted into multiple sub packages and If we could receive some helps from the community, it wont be such not possible.

mbostock commented 6 years ago

To set clear expectations: I do not anticipate that I will ever switch to TypeScript. I could imagine adopting types if they become a native feature of ECMAScript, but I won’t speculate as to how likely that is.

I’ve had a good experience working with @tomwanzek—together we’ve fixed some ambiguities in the API and documentation—and so if you want my involvement, I suggest something along the lines of what we’ve already been doing. I’m happy to review PRs and issues around documentation and API behavior. This work is beneficial to all users of D3. I apologize for not being able to offer further assistance in this area, but I have other responsibilities that compete for my time.

I’m going to close this issue because I have no immediate plans to undertake these proposals (to adopt TypeScript & migrate to monorepo). But you are welcome to continue to discuss here how to achieve the desired goals and I’ll offer help where I can. Thank you.