erikbrinkman / d3-dag

Layout algorithms for visualizing directed acyclic graphs
https://erikbrinkman.github.io/d3-dag/
MIT License
1.45k stars 87 forks source link

Publish types #45

Closed Trias closed 3 years ago

Trias commented 4 years ago

Thanks for rewriting d3-dag in typescript! I'm starting to use your lib & it looks very promising!

It would be helpful to publish types, so users of the lib can get ide support.

Here is a guide: https://www.typescriptlang.org/docs/handbook/declaration-files/publishing.html

Thank you :)

erikbrinkman commented 4 years ago

This is my first typescript library, and the interoperability between all the different javascript flavors is not super obvious to me, but if you're consuming typescript, I intended you to use the modularized format, e.g. instead of import { dagConnect } from "d3-dag" I intended you to use import { connect } from "d3-dag/src/dag/connect".

I tested this (now), and it seems to mostly work, however, I had to patch in types for two external libraries, and it's not clear exactly how to pass those shims along. They're currently not even packaged, so that's clearly an issue, but I'm not actually sure of the best way to actually include them is. e.g. if you can point to more specific information, that'd be very helpful!

Trias commented 4 years ago

Well, I'm not an expert myself. I hope you would ;). I guess we are all learners.

First thing I learned: i'm apparently using your library the wrong (or old way). Basically it looked like this: import * as d3core from "d3"; import * as d3dag from "d3-dag"; const d3 = Object.assign({}, d3core, d3dag);

which is based on the examples. I realize, after looking at the source that you can import functions from d3-dag directly.

still, it gives me an error in vscode as it can't find the right types: image

I tested @jordyLangen PR by manually compiling and copying. I had to add "types": "./typings/index.d.ts" to get it working. Still it does not seem complete, as it only exports typings for the main module: image

It does work when you import the src directory as you described but i guess thats uncommon image

what did work as I expected was using https://www.npmjs.com/package/npm-dts it generates an index.d.ts with all types and start-import and deep import worked as expected. I added the index.d.ts: index.d.ts.zip image

I honestly don't know what the best way is. they all seem in some way cumbersome.

Xylez01 commented 4 years ago

Well, I'm not an expert myself. I hope you would ;). I guess we are all learners.

I've never published a package to npm before, so sadly not hehe. I did something similar to you, I checked out the source code locally and generated the typings.

From what I understood, having a types or typings dir in the root directory of your npm module should do the trick.

"types": "./typings/index.d.ts"

You added this locally in your project, referencing the generated index.d.ts?

Trias commented 4 years ago

You added this locally in your project, referencing the generated index.d.ts?

no i added it to the package.json of d3-dag

I found the following tutorial which i think looks good: https://hackernoon.com/building-and-publishing-a-module-with-typescript-and-rollup-js-faa778c85396

Some things that I found out:

  1. a lib should include a "main" entry point (for common.js aka "require()") and a "module" (for ecmascript-modules aka "import")
  2. with a module entry point you cannot require source files directly, meaning you have to define the public interface (including types) in your index.js only when you specify "exports", see https://nodejs.org/api/packages.html#packages_package_entry_points
  3. if you want multiple entrypoints in your es-module, you can define it in the package.json like described here: https://stackoverflow.com/questions/63058081/package-json-with-multiple-entrypoints (with "exports")

It kind of works the way you describe with requiring "d3-dag/src/..." but that's because the typescript files are included in the package and typescript includes them in its build.

This may become a problem when the settings are different and forces every consumer to redeclare the typings you declared in /types. Also I'm currently getting 236 errors when using a "vanilla-ts"-project (tsc --init), because you need compatible "lib" and "target" settings in your tsconfig.json.

I'v added a pull request which publishes an esm build and types, so you import the from the file at "dist/.." and not "src/.." this way typescript only looks at the .d.ts for its typings and you don't need compatible settings and/or transitive typings.

hadrienk commented 4 years ago

Looking forward to this. I'm writing a React component that wraps your lib and it will allow to enforce correct types in props.

natrayanp commented 4 years ago

Previously D3-DAG worked well in angular but now the build fails with error. May you consider this in the fix so it works with Angular as well

Angular Package Format (https://goo.gl/jB3GVv).

Angular ERROR DETAILS BELOW:

ERROR in ./node_modules/d3-dag/src/index.ts Module build failed (from ./node_modules/@ngtools/webpack/src/index.js): Error: /home/projects/ETL/newdrawnode/node_modules/d3-dag/src/index.ts is missing from the TypeScript compilation. Please make sure it is in your tsconfig via the 'files' or 'include' property. The missing file seems to be part of a third party library. TS files in published libraries are often a sign of a badly packaged library. Please open an issue in the library repository to alert its author and ask them to package the library using the Angular Package Format (https://goo.gl/jB3GVv). at AngularCompilerPlugin.getCompiledFile (/home/projects/ETL/newdrawnode/node_modules/@ngtools/webpack/src/angular_compiler_plugin.js:949:23) at plugin.done.then (/home/projects/ETL/newdrawnode/node_modules/@ngtools/webpack/src/loader.js:43:31) at process._tickCallback (internal/process/next_tick.js:68:7)

erikbrinkman commented 4 years ago

Just updated a version with hopefully better support for types. There were a number of people with different issues, and I want to make sure that those different issues are resolved before closing this out.

natrayanp commented 4 years ago

Just updated a version with hopefully better support for types. There were a number of people with different issues, and I want to make sure that those different issues are resolved before closing this out.

Thank you. Build successful with 0.4.6 version (in Angular 10).

cllu commented 3 years ago

Hi @erikbrinkman, great work on adding type support. I'm wondering if it's ok to export the types in src/dag/node, such as LayoutChildLink and DagNode. The DagNode would be essential to create custom data class in my use case. I have tested locally and add export * from "./dag/node"; to the src/index.ts file is enough to get all types exported in the build.

erikbrinkman commented 3 years ago

@cllu As per the discussion in this group, I'm hesitant to export all of the types because of the pollution of the namespace since there's only one. Ideally a solution to types that would allow all of the modules would be idea. Since this doesn't seem generally or easily supported in the short term, I'm okay with exporting some types, although I want to be careful of hurting versioning by exporting too much, and then having to make a breaking change, although I'm not sure if it matters too much whether the interfaces are exposed.

That being said, right now, nothing is exposed, and it seems to make sense to export at least Dag, DagNode, DagRoot, Link, and since you suggested the layout version, probably ChildLink. Would that be enough to suit your needs?

cllu commented 3 years ago

I see, I understand your concern now. I'm trying to collapse/uncollapse the DAG graph by mutating the LayoutDagNode.dataChildren property and then request a new layout. The way I did it is to recreate the elements in dataChildren given its children nodes on uncollapsing, thus requiring access to the LayoutChildLink constructor. I'm also trying to modify the default output of ConnectOperator to prefer root node to always be a LayoutDagRoot, thus requiring access to it as well. I'm not sure if this is the best way to do it. Do you have recommendation on that?

erikbrinkman commented 3 years ago

A lot of this comes from the fact that you're trying to mutate the dag itself every time, which isn't really supported, or hasn't been historically part of the api. It's not something that d3's hierarchy seems to really support either, although the collapsible example currently does it. Is there a problem with doing it the way d3 does? e.g. layout the full dag, then mask out some of the children? Another alternative would be to just internally mark them as hidden, and adjust the node width in layout.

I'm open to making this more flexible, but I want to do it, "the right way," and I'm wondering first if there are better approaches / alternatives to achieving a collapsible dag layout.

erikbrinkman commented 3 years ago

The latest version has export { Dag, DagNode, DagRoot, Link, ChildLink } from "./dag/node"; in the index file, hopefully that helps, but I'm still open to publish more of the types externally.

erikbrinkman commented 3 years ago

While the type publishing could certainly be better, some of that stems from typescripts support for publishing types right now. Is there anything more you wish to see from the published types? If not, I'm going to close this out.

Trias commented 3 years ago

I also use the SugiyamaNode-Type here https://github.com/erikbrinkman/d3-dag/blob/master/src/sugiyama/index.ts#L57-L61

Thank you for your support!

erikbrinkman commented 3 years ago

yeah, that makes sense. It's exported in 0.6.1

erikbrinkman commented 3 years ago

closing as this seems be addressed. Let me know if there are other issues