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

Add TS typings #2102

Merged
merged 1 commit into from
Aug 21, 2019
Merged

Add TS typings #2102

merged 1 commit into from
Aug 21, 2019

Conversation

IvanGoncharov
Copy link
Member

@IvanGoncharov IvanGoncharov added the PR: feature 🚀 requires increase of "minor" version number label Aug 21, 2019
@TonyPythoneer
Copy link

TonyPythoneer commented Aug 21, 2019

Hi @IvanGoncharov

Sorry, I have to click downvote.

This is my personally disagree.

I hope everyone listens to my opinion. Thanks.

To summary, this PR never explains that graphql team how to face TS world and management strategy if graphql team accepts TS definition will get merged. I know transferring source code from somewhere to somewhere is not illegal if someone or an organization with strong enthusiasm to make it better, it's very ideal. I believe everyone will strongly agree.

Therefore, I need a representative of graphql team states that you have a strong determination to embrace TS. Do we believe you can make it better than it's on https://github.com/DefinitelyTyped/DefinitelyTyped community.

Because we are in the best Open Source Community, I treat this communication and interaction seriously.


Originally, TS's maturity is not the same as now. Most of libs don't have TS definition. They are stored in https://github.com/DefinitelyTyped/DefinitelyTyped . It's welcome that TS lovers commit a number of contributions. That makes early TypeScript have a stable bridge and relationship with JavaScript. It brings so much convenience that TS developers fast and strictly code JS Application based on the definition. Owing to JS can't understand TS's advantage in the past, they are individually maintaining code in different repositories.

Why did I say? I started to touch TypeScript from 1.x. TypeScript grows fast until most of front-end developers accept it as the main or popular programming language. I'm really touched.

As far as I know, https://github.com/DefinitelyTyped/DefinitelyTyped hope the definition should return their original repository. It should be maintained by the repository ecosystem. That's good news for me.


Finally, this is my small requirement: I hope the record https://github.com/DefinitelyTyped/DefinitelyTyped/blob/54712a7e28090c5b1253b746d1878003c954f3ff/types/graphql/index.d.ts#L1-L22 doesn't get erased their contribution record. It's a historical meaning for us. Thanks

How do you think? The TS contributors.
@TonyPythoneer @calebmer @intellix @firede @kepennar @freiksenet @IvanGoncharov @DxCx @rportugal @tgriesser @dyst5422 @adnsio @divyenduz @bradzacher @clayne11 @JCMais @langpavel @mc0 @martijnwalraven @jedmao

@IvanGoncharov
Copy link
Member Author

@TonyPythoneer What do you mean by graphql team?
Here is the list of contributors: https://github.com/graphql/graphql-js/graphs/contributors
I'm volunteering to support this TS typings with help of community so I don't see any problems here.

@IvanGoncharov IvanGoncharov merged commit d1391ee into graphql:master Aug 21, 2019
@TonyPythoneer
Copy link

TonyPythoneer commented Aug 21, 2019

@IvanGoncharov
Sorry, I just acted emoji to express my emotion before typing.
It's because English is not my native language, I need time to type.

If you're available, please read my reply. thx.
#2102 (comment)

@bradzacher
Copy link

From my POV - I could care less.

It's not like the history has been completely omitted - the merge commit was created with the same description as this PR: d1391ee
Which means that looking at the file's history, you will be able to follow the link to the original file within the DefinitelyTyped repo. Additionally all the contributors are directly tagged in the commit.

@IvanGoncharov
Copy link
Member Author

@TonyPythoneer Sorry, I didn't refresh this issue before merging so I didn't saw you updated your comment. Also not sure if people are notified since GitHub sent email only then you post a comment not than you update it.

@TonyPythoneer Even though code in question is under MIT license (same as this library) if you or any other author oppose your code being merged I can revert this commit and rewrite it from scratch.
I believe the end result would be pretty similar since typings are based on Flow types and JSDoc comments.

@intellix
Copy link

Awesome stuff! Just yesterday I noticed DefinitelyTyped was missing the new utility trimIgnoredCharacters :)

@TonyPythoneer
Copy link

@IvanGoncharov You can send new PR to add the authors info. It’s okay.

@TonyPythoneer
Copy link

BTW @IvanGoncharov
Please note ti will have more tasks need to do.

  1. Tell admin of DefinitelyTyped team that the typing can get removed because it has migrated.
  2. Tell other contributors on DefinitelyTyped who are actively in that. They should stop sending PR to DefinitelyTyped. In the future, it should focus on here.

This is my opinions.

@IvanGoncharov
Copy link
Member Author

IvanGoncharov commented Aug 21, 2019

You can send new PR to add the authors info. It’s okay.

@TonyPythoneer We don't have such list in any other file so I moved it to README please see #2103

@IvanGoncharov
Copy link
Member Author

BTW @IvanGoncharov
Please note ti will have more tasks need to do.

Tell admin of DefinitelyTyped team that the typing can get removed because it has migrated.
Tell other contributors on DefinitelyTyped who are actively in that. They should stop sending PR to DefinitelyTyped. In the future, it should focus on here.
This is my opinions.

@TonyPythoneer Yes definitely. After I release 14.5.0 I plan to follow the standard algorithm for removing the package from DefinetlyTyped: https://github.com/DefinitelyTyped/DefinitelyTyped#removing-a-package

@TonyPythoneer
Copy link

Okay, It looks good to me. thx

@langpavel
Copy link
Contributor

langpavel commented Aug 27, 2019

I'm happy that TS definitions are now part of this library,
but I'm not so happy with the way it's done. Feels a little like stealing. I can see @TonyPythoneer point of view — I'm participant to both, graphql-js and DT types.

What I miss is more transparent strategy for future contributions. How TS typings will be enforced?

By merging, but not adopting — you @IvanGoncharov — take responsibility for type definitions.

  • Do you have strategy for adopting typescript?
  • Shall we expect transition of graphql-js to TS?
  • What about flow? (I'm not worry, but someone can?)

BTW I think that this is good step forfard.

@jednano
Copy link

jednano commented Aug 28, 2019

Do you personally feel stolen from? If not, why bring this up? It's definitely not stealing. It's the most ideal way to distribute non-generated types without requiring an additional installation. It only benefits the TypeScript ecosystem and I'm positive the type authors 100% welcome a better TS ecosystem, as that's why they contributed in the first place.

Merging types into a repo, as has been done with many other repos, does not at all indicate a desire to adopt TypeScript source code. Authors of open source projects have no actual responsibility. You should be grateful if they maintain it, let alone contribute fresh types to it.

Why are you asking about Flow? Do you actually need these features in 2019 or are you asking for some hypothetical situation that isn't even in high demand? Flow has a grim future. It's days are numbered.

All that said, the only proper way to provide first class TypeScript support is for the types to be generated from TS source files. If that's not the goal, and/or if you're not personally going to help in that transition, I think the best etiquette is to stand back and let the author + contributors contribute at their own pace and within their own comfort levels and environment.

Thanks to all who have contributed to this and other open source projects. You do you.

@IvanGoncharov
Copy link
Member Author

I'm happy that TS definitions are now part of this library,
but I'm not so happy with the way it's done. Feels a little like stealing.

@langpavel I'm confused about "stealing" part. Both commit and PR have clear attribution and link to the original source also on @TonyPythoneer request I added credits to README see #2103

Since legally this repository belongs to GraphQL Foundation (I'm not a member, just a person who got commit rights in this repo) I don't want to make foundation responsible for my decisions.
Even though *.d.ts files were distributed under MIT, if you or any other the authors don't want them to be included in this repo I can revert entire PR or alternative only specific parts that you contributed:
image

@langpavel Can you please respond on how this situation can be resolved?

By merging, but not adopting — you @IvanGoncharov — take responsibility for type definitions.

I always take responsibility for everything that I merge or commit to this and other repos that I maintain. Even if PR comes from other developers I still take personal responsibility for stuff that I merge.

That said I can't guarantee the enterprise level of support since I'm maintaining this library in my own free time. But I always try to respond on issues and PRs within a day and if I disagree with the proposed fix I always try to come up with an alternative in the reasonable amount of time.

Do you have strategy for adopting typescript?

At the time of this PR, I was still in the process of discussing it with GraphQL Foundation.
After I got the green light from GraphQL Foundation I started a public discussion in #2104 (Please keep this discussion technical and connected to the actual plan)

What about flow? (I'm not worry, but someone can?)

We will continue to provide Flow typings as part of this repository, please see #2104

@langpavel
Copy link
Contributor

I'm really sorry about stealing word. It may sounds much harder than I wish…

@victortwc
Copy link

victortwc commented Aug 28, 2019

Requires typescript 3+ and breaks projects with old typescript support.
Isn't is better to be optional and leave this to @types/graphql ?

@mortenbroesby
Copy link

+1 on making the types be optional, using @types/graphql

@IvanGoncharov
Copy link
Member Author

I'm really sorry about stealing word. It may sounds much harder than I wish…

@langpavel Thanks for clarification 👍 I have no hard feelings. Glad it resolved.
Sorry for being too formal but I'm really worried about the legal side of things since I'm not an owner of this project.

At the same time, I think we need to be more transparent with the roadmap and discuss it more with the community.

Requires typescript 3+ and breaks projects with old typescript support.

@victortwc At some point even DefinetlyTyped will be forced to switch to typescript 3+
In the case of graphql-js we will continue to maintain 14.5.x line until v16 releases (after Jan 2020, to drop Node8).

Having separate @types/graphql will create a lot of confusion and will also split maintaining efforts from the community. But more importantly, we need to switch TS to prevent community split since I see more and more attempts to rewrite graphql-js in TypeScript, e.g. here and here.
We either rewrite graphql-js in TS or we risking to have community split between graphql-js and it's TS fork.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: feature 🚀 requires increase of "minor" version number
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants