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

Typescript #25

Merged
merged 40 commits into from
Aug 16, 2019
Merged

Typescript #25

merged 40 commits into from
Aug 16, 2019

Conversation

gr2m
Copy link
Contributor

@gr2m gr2m commented May 20, 2019

ping @kevinpollet for updates and review when @wolfy1339 is ready

@gr2m

This comment has been minimized.

@gr2m gr2m closed this May 20, 2019
@gr2m gr2m reopened this May 20, 2019
@gr2m gr2m mentioned this pull request May 20, 2019
@wolfy1339 wolfy1339 force-pushed the typescript branch 2 times, most recently from ad412a5 to 68c7664 Compare May 22, 2019 19:33
@wolfy1339
Copy link
Member

This is pretty much in-line with the other octokit packages. I believe it's ready for a review

@wolfy1339 wolfy1339 marked this pull request as ready for review May 22, 2019 21:42
@gr2m gr2m self-assigned this May 22, 2019
@gr2m
Copy link
Contributor Author

gr2m commented May 22, 2019

npm run build is currently throwing for me

> pack build

@pika/pack build v0.3.7
[1/8] Validating source...
[2/8] Preparing pipeline...
      ❇️  pkg/
      » tsconfig.json [compilerOptions.target] should be "es2019", but found "5". You may encounter problems building.
[3/8] Running @pika/plugin-ts-standard-pkg...
Error: Command failed: /Users/gregor/Projects/octokit/graphql.js/node_modules/.bin/tsc --outDir /Users/gregor/Projects/octokit/graphql.js/pkg/dist-src/ -d --declarationDir /Users/gregor/Projects/octokit/graphql.js/pkg/dist-types/ --project /Users/gregor/Projects/octokit/graphql.js/tsconfig.json --declarationMap false --target es2019 --module esnext

src/graphql.ts(39,5): error TS2345: Argument of type '{}' is not assignable to parameter of type 'Endpoint'.
  Type '{}' is missing the following properties from type '{ method: Method; url: string; }': method, url
src/index.ts(4,25): error TS2732: Cannot find module '../package.json'. Consider using '--resolveJsonModule' to import module with '.json' extension

    at makeError (/Users/gregor/Projects/octokit/graphql.js/node_modules/@pika/plugin-ts-standard-pkg/node_modules/execa/index.js:174:9)
    at Promise.all.then.arr (/Users/gregor/Projects/octokit/graphql.js/node_modules/@pika/plugin-ts-standard-pkg/node_modules/execa/index.js:278:16)
    at process._tickCallback (internal/process/next_tick.js:68:7)

package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
.travis.yml Outdated Show resolved Hide resolved
@wolfy1339
Copy link
Member

wolfy1339 commented May 22, 2019 via email

@wolfy1339 wolfy1339 force-pushed the typescript branch 2 times, most recently from 0f55540 to 59fc388 Compare May 22, 2019 23:18
Copy link
Contributor Author

@gr2m gr2m left a comment

Choose a reason for hiding this comment

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

oops looks like I started a review but never submitted it.

What’s left?

.travis.yml Outdated
@@ -27,28 +27,23 @@ jobs:
env: Node 10, coverage upload
Copy link
Contributor Author

Choose a reason for hiding this comment

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

remove the env: key, it’s no longer needed. I use it as label but now we just test Node 10

@wolfy1339
Copy link
Member

wolfy1339 commented May 27, 2019 via email

@gr2m
Copy link
Contributor Author

gr2m commented May 28, 2019

Yeah this is tricky. I spent some time on it but to no avail. It feels like Typescript is really getting into the way of a simple implementation here.

I wonder if it would be easier to have all type definitions within the repository for now (remove the require('@octokit/request/dist-types/types')), don’t worry about duplication. It might be easier to debug and ask Typescript folks for help when everything is self contained.

I think down the road I’d like to move all typescript definitions for @octokit/* reposistories into a single repo anyway, we can take care of deduplication then

src/error.ts Outdated
import { GraphQlQueryResponse } from "./types";

export class GraphqlError<T extends GraphQlQueryResponse> extends Error {
public request: Request;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

error.request is not the @octokit/request method, but request options (what @octokit/endpoint returns, an object with just method, url, headers and optionally body and request

@giovannidegani
Copy link

Any updates on this ? it would be really good to have TS for this lib.

@gr2m
Copy link
Contributor Author

gr2m commented Jul 5, 2019

@giovannidegani you can look into the remaining open issues

you can send a pull request against this branch if you like

@wolfy1339
Copy link
Member

I have read through previous conversations, and I agree, let's deal with de-duplication later and do whats needed to get this working and make it easier

@gr2m
Copy link
Contributor Author

gr2m commented Aug 15, 2019

👋 I'm looking into this now and will try to finish it up, as it blocks octokit/core.js#3

@gr2m

This comment has been minimized.

@gr2m
Copy link
Contributor Author

gr2m commented Aug 15, 2019

As far as I can see the only breaking change is the named export

const { graphql } = require('@octokit/graphql')

instead of

const graphql = require('@octokit/graphql')
```

Which aligns with the other modules we converted to Typescript/ES Modules using Pika

@gr2m
Copy link
Contributor Author

gr2m commented Aug 16, 2019

I ran the built version against the current test in master and they all passed, so I feel confident to ship this ✌️ :shipit:

@gr2m gr2m merged commit c7c3200 into master Aug 16, 2019
@wolfy1339 wolfy1339 deleted the typescript branch August 16, 2019 04:27
@octokitbot
Copy link
Collaborator

🎉 This PR is included in version 4.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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.

5 participants