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

Proposal: Introduce typescript migration pipeline #352

Merged
merged 1 commit into from
Jan 31, 2018
Merged

Proposal: Introduce typescript migration pipeline #352

merged 1 commit into from
Jan 31, 2018

Conversation

kwonoj
Copy link
Contributor

@kwonoj kwonoj commented May 18, 2017

  • I've read and understood the Contributing guidelines and have done my best effort to follow them.
  • I've read and agree to the Code of Conduct.
  • I've been mindful about doing atomic commits, adding documentation to my changes, not refactoring too much.
  • I've a descriptive title and added any useful information for the reviewer. Where appropriate, I've attached a screenshot and/or screencast (gif preferrably).
  • I've written tests to cover the new code and functionality included in this PR.
  • I've read, agree to, and signed the Contributor License Agreement (CLA).

PR Summary

This PR is opposite approach of #329, instead of introducing ambient type definition which requires manual handcraft of types directly migrates codebases into TypeScript in gradual way.

While this PR doesn't actually migrates lot of codes into TypeScript yet (only except index entry point), allows let TypeScript compiler works with existing JS codebases as well as TypeScript both for gradual migration with community contributions as well.

What it does

  • Setting up dev pipeline for TypeScript compiler, having compatibility to existing JS codebases
  • Setting up Test, Coverage, Lint pipeline for TypeScript codes
  • Setting up git hooks to prevent non-linted, non-compilable code to be pushed into repo
  • Code will be type-guarted as soon as it is migrated
  • Type definition is synced to actual implementation

What it doesn't (yet)

  • Producing actual type definition
    : TypeScript compiler doesn't support to generate type declaration while accepting JS (--allowJS).
    Once migration is done actual type declarattion can be published

What it breaks

  • Documentation pipeline
  • Linting rules
    : TSLint rules are not 100% 1:1 correponds to ESLint rules.
  • (Possibly) Node 0.12 compatibility
    : Tooling around TypeScript compiler generally targets Node > 4, so test might be skipped on 0.12 environment which can cause breaking changes to 0.12.

Related Issues

Relates to #329

Test strategy

Nothing, this is just initial discussion proposal and won't be likely merged by some breaking changes.

@CLAassistant
Copy link

CLAassistant commented May 18, 2017

CLA assistant check
All committers have signed the CLA.

@codecov
Copy link

codecov bot commented May 18, 2017

Codecov Report

Merging #352 into dev-v4 will increase coverage by 10.79%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           dev-v4     #352       +/-   ##
===========================================
+ Coverage   88.97%   99.77%   +10.79%     
===========================================
  Files          44       24       -20     
  Lines        1234      900      -334     
  Branches      183        0      -183     
===========================================
- Hits         1098      898      -200     
+ Misses        136        2      -134
Impacted Files Coverage Δ
lib/helpers.js 100% <0%> (ø) ⬆️
lib/models/file.js
lib/data-store/message-handlers/helpers.js
lib/data-store/message-handlers/user.js
lib/clients/transports/request.js
lib/models/mpdm.js
lib/models/index.js
lib/data-store/message-handlers/message.js
lib/clients/errors.js
lib/models/user.js
... and 57 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e1014fb...bd18ca2. Read the comment docs.

sampsyo added a commit to cucapra/opal-bot that referenced this pull request May 31, 2017
Can't find any up-to-date types for @slack/client. Looks like they're
transitioning to TS, though:
slackapi/node-slack-sdk#352
@aoberoi
Copy link
Contributor

aoberoi commented Jun 1, 2017

one constraint i'd have for a migration to typescript-authored source is that i'd want that fact to remain an implementation detail. a goal of this project is to have maximum reach with our developers and therefore, all the examples / guides / documentation should talk about the interfaces as JavaScript-first.

here's a question related to this: how do we go about generating reference documentation? its clear that JSDoc annotations are the infrastructure for nearly all tools in the ecosystem, and i'd love for reuse. while investigating a solution, i came across microsoft/TypeScript#10. it seems like this is not a solved problem, so i'm open to other solutions as well.

more interesting links:

@kwonoj
Copy link
Contributor Author

kwonoj commented Jun 2, 2017

@aoberoi I've peek into existing documentation (i.e: https://slackapi.github.io/node-slack-sdk/reference/SlackDataStore), and I don't think there is strong differences between documentation generated by typedoc (https://github.com/TypeStrong/typedoc).

In a high level, there could be possibly few differences -

  • Object can be detailed interface or types
    for example, think of simple function like () => { a: 1}; then typedoc will represent return type as {a: string}. This is just enhanced represenation of object as similar to other (ESDoc, i.e) can do as well.

  • Generics can appear documentation
    functions like listify(arr: Array<T>, conj?: "or" | "and"): Array<T> will still have generic representation in documentation. As plain javascript doesn't have concept of generics, this is one thing could possibly different to plain JS based documentation, but I guess this can be sufficiently explained by supplied doc comment to each generic means.

My suggestion in here is not to limit options to transform TS codes to JS-doc-compatible but try out other ooto documentation as well. If those results are highly different to plain JS then considering transformation should be way to go.

@kwonoj
Copy link
Contributor Author

kwonoj commented Jun 2, 2017

Also in addition to ng's tsickle, it's worth to visit ng's documentation tool itself (https://github.com/angular/angular/tree/master/aio), which generates documentation like https://angular.io/docs/ts/latest/api/http/index/Http-class.html . Those tool internally uses Dgeni (https://github.com/angular/dgeni). Dgeni itself is highly configurable, so this would be probably best solution if we want customized documentation for specific purposes.

@aoberoi
Copy link
Contributor

aoberoi commented Jun 19, 2017

@kwonoj i like your idea about trying out tsdoc and then inspecting whether it suits the needs of a JavaScript only developer. we can refer back to this conversation to try out other options when we know what worked and what didn't.

@kwonoj
Copy link
Contributor Author

kwonoj commented Jun 19, 2017

@aoberoi yes, makes sense. Let's try to find out if things are not working with tsdocs.

@aoberoi
Copy link
Contributor

aoberoi commented Jun 19, 2017

@kwonoj so i've gone ahead and created a dev-v4 branch. would you mind opening this PR against that branch so we can start merging the pieces that work and continue to collaborate on this (along with other v4 goals)? i'm going to start publishing a roadmap in the form of GitHub Milestones, and we can track the progress there.

@kwonoj
Copy link
Contributor Author

kwonoj commented Jun 19, 2017

absolutely ✨ I'll prep changes.

@kwonoj kwonoj changed the base branch from master to dev-v4 June 20, 2017 17:06
@kwonoj
Copy link
Contributor Author

kwonoj commented Jun 20, 2017

Rebased, and target base changed to dev-v4.

@aoberoi aoberoi mentioned this pull request Oct 3, 2017
3 tasks
Copy link
Contributor

@aoberoi aoberoi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gate for proposal

@aoberoi aoberoi added this to the v4.0.0 milestone Dec 22, 2017
@aoberoi aoberoi merged commit bd18ca2 into slackapi:dev-v4 Jan 31, 2018
@aoberoi
Copy link
Contributor

aoberoi commented Jan 31, 2018

@kwonoj thanks for these very excellent contributions. i'm very happy to say we're rolling forward with the typescript pipeline and this really helps establish a base. of course i might tweak some of the details ahead.

@kwonoj kwonoj deleted the feat-typescript branch January 31, 2018 18:24
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