adrai / flowchart.js

Draws simple SVG flow chart diagrams from textual representation of the diagram
http://flowchart.js.org/
MIT License
8.49k stars 1.21k forks source link

added types #212

Closed DerMolly closed 3 years ago

DerMolly commented 3 years ago

As part of HedgeDoc we wrote these type definitions to use this library, but we think they would have a far better home in this repository.

adrai commented 3 years ago

Since I'm not a TypeScript user, would you be open to maintain the types for flowchart.js if I merge this PR?

DerMolly commented 3 years ago

As I intend to use them, yes, but I'm not sure these types cover everything the lib can do. Currently it only covers the parse function and it's parameter and output

adrai commented 3 years ago

It's more, if there's a GitHub issue concerning TypeScript, you could look at it. What do you think?

DerMolly commented 3 years ago

Feel free to assign me those. I'll try to catch them myself if I can.

adrai commented 3 years ago

awesome, thx

adrai commented 3 years ago

included in v1.14.2

Mister-Hope commented 3 years ago

The types has tons of problems @DerMolly . You broke my whole environment. You should make a full test before submitting this PR.

First of all, the drawSVG funtion supports both id and Element,

So

drawSVG: (container: HTMLElement, options: Options) => void,

should be:

drawSVG: (container: HTMLElement | string, options: Options) => void,

Also There are tons of options

image

So what's this? Are you kidding me? Are these just what you use in your own project? This is not your own package bro! :anger:

  export type Options = {
    'line-width': number,
    'fill': string,
    'font-size': string,
    'font-family': string
  }

And the most important thing is you should use interfaces, not types, in order to let other usesr extend them in their own ts files.

If you write like this:

  export interface Options  {
     // ...
  }

Then users like me should be able to extend the interface rather than making this comment with anger.


declare module "flowchart.js" {
  interface Options  {
     // ...
  }
}

The author do not have any faults, since he don't know typescript, but you should think about your behavior, sending this joke PR. If you are typescript newcomers, you should ask for help, rather than writing a few lines just to met your needs and make this PR without full test! ๐Ÿ’ข I have a pakage using flowchar.js with ^1.14.1 and your PR is inflecting all my users.


I will make a PR to fix this later.@adrai But I suggest you publish 1.14.3 to revert this PR.

Adding types should be breaking changes, if you don't want to raise to V2, at least you should publish has 1.15.0. I believe there are a lot of users who will write their own typescript declaration files. And the official type files must be conflicted with some peoples's files.

Mister-Hope commented 3 years ago

I make no offense, I won't be anger if there are a few mistake in this declaration files, but the file you create 's sytax really has a lot to problems, making me feel you are a newcomers for typescript. Also, lack of usaging checking and the poor Option type really make me feel this is a joking PR. If you ever checked the website, the Option type shouldn't be that simple.

DerMolly commented 3 years ago

Sorry to have caused you problems. I honestly just wanted to share what we wrote for our project as I belive that any types are better than none, but I can understand your frustration. Do you have time to fix that now or should I do it?

Mister-Hope commented 3 years ago

I am rebuilding types files, and I will make a PR later. ๐Ÿ˜ƒ I clam down now. Hahaha Just be carefully and make a full test before making PRs (โยดโ—ก`โ)

DerMolly commented 3 years ago

Looking forward to your PR then

adrai commented 3 years ago

If it is better to not have any types, I would just revert that PR and never include any type.

Just let me know.

Mister-Hope commented 3 years ago

It's better to include types, just like other repos, but the type should be fully checked. Also, at least the minor version code should be changed. So could you release 1.14.3 to revert this PR, and wait for my PR to release 1.15.0 including types again?

Since flowchart is really lack of documentation, I would prefer reading through the codes and make full checking before making the final PR. It will take some time, maybe one day or two prehaps.

Releasing 1.14.3 to revert this PR will make sure nobody writing like ^1.14.0 will be inflected.

Mister-Hope commented 3 years ago

By the way, is all options including in https://github.com/adrai/flowchart.js/blob/master/src/flowchart.defaults.js @adrai

adrai commented 3 years ago

should be, yes ( it's a really old code base ;-) )

adrai commented 3 years ago

Ok, I've published a new version v1.14.3