Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix discrepancy between TypeScript definition and acutal module export #6

Closed
wants to merge 2 commits into from

Conversation

mrkev
Copy link

@mrkev mrkev commented Jan 28, 2022

Fixes #5 to allow Graph to be properly imported on TypeScript projects

@ChristopherChudzicki
Copy link

@mrkev I just ran into this issue, too. Thanks for the PR pointing out the discrepancy.

It might be better to fix the typings rather than the change the export, though. This PR would be a breaking change.

For others stumbling here... to monkey patch the types so they accurately reflect the tarjan-graph library:

declare module "tarjan-graph" {
  interface Vertex {
    name: string;
    successors: Vertex[];
    index: number;
    lowLink: number;
    onStack: boolean;
    visited: boolean;
    reset: () => void;
  }

  export default class Graph {
    vertices: { [key: string]: Vertex };
    add(key: string, descendants: string[] | string): Graph;
    reset(): void;
    addAndVerify(key: string, descendants: string[] | string): Graph;
    dfs(key: string, visitor: (v: Vertex) => void): void;
    getDescendants(key: string): string[];
    hasCycle(): boolean;
    getStronglyConnectedComponents(): Vertex[][];
    getCycles(): Vertex[][];
    clone(): Graph;
    toDot(): string;
  }
}

@tmont tmont closed this in 3667d88 Jan 31, 2022
@tmont
Copy link
Owner

tmont commented Jan 31, 2022

Hi. Thanks for the PR and the comments about how terribly lazy I've been with the TypeScript stuff. I decided to just port the whole thing to TypeScript and emit the declaration that way instead of (poorly) handcrafting it. This should fix all problems with the code not matching the declarations. Sorry about that.

The updated module is in NPM at tarjan-graph@3.0.0. Note that this is in fact a breaking change as I changed the default export as both of you noticed. So now your code should be like:

const Graph = require('tarjan-graph').default; // js
import Graph from 'tarjan-graph'; // ts

@mrkev
Copy link
Author

mrkev commented Feb 4, 2022

Hey sorry just saw this. haha you're right, changing the definition would've been better @ChristopherChudzicki. In any case thanks for figuring this out @tmont 👍

@ChristopherChudzicki
Copy link

Thanks, @tmont!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Getting Webpack error on "new Graph() in ts file
3 participants