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

added types #212

Merged
merged 1 commit into from
Oct 11, 2020
Merged

added types #212

merged 1 commit into from
Oct 11, 2020

Conversation

DerMolly
Copy link
Contributor

@DerMolly DerMolly commented Oct 11, 2020

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
Copy link
Owner

adrai commented Oct 11, 2020

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

@DerMolly
Copy link
Contributor Author

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
Copy link
Owner

adrai commented Oct 11, 2020

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

@DerMolly
Copy link
Contributor Author

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

@adrai adrai merged commit d09ee11 into adrai:master Oct 11, 2020
@adrai
Copy link
Owner

adrai commented Oct 11, 2020

awesome, thx

@DerMolly DerMolly deleted the types branch October 11, 2020 13:49
@adrai
Copy link
Owner

adrai commented Oct 11, 2020

included in v1.14.2

@Mister-Hope
Copy link
Contributor

Mister-Hope commented Oct 12, 2020

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! 💢

  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
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Contributor Author

Looking forward to your PR then

@adrai
Copy link
Owner

adrai commented Oct 12, 2020

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
Copy link
Contributor

Mister-Hope commented Oct 12, 2020

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
Copy link
Contributor

@adrai
Copy link
Owner

adrai commented Oct 12, 2020

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

adrai added a commit that referenced this pull request Oct 12, 2020
This reverts commit d09ee11.
@adrai
Copy link
Owner

adrai commented Oct 12, 2020

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

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.

3 participants