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

Convert project to typescript #45

Merged
merged 46 commits into from
May 15, 2019
Merged

Convert project to typescript #45

merged 46 commits into from
May 15, 2019

Conversation

wolfy1339
Copy link
Member

@wolfy1339 wolfy1339 commented May 1, 2019

Had this lying around, and now that @octokit/endpoint was converted, this is the next one down the pipe

Todos

Resolve as part of this PR or make follow up issues

@wolfy1339
Copy link
Member Author

@gr2m I've mostly finished this, now would be the time you come by and fix the remainder of the wrnings with your knowledge of the codebase

@gr2m
Copy link
Contributor

gr2m commented May 1, 2019

see my changes at octokit/endpoint.js#47 introducing pika for building / direct import for modern browsers from https://unpkg.com/

I’ll work on typescript definitions for all endpoints, at least for the endpoint(route, options) interface, I’d like to add the same to @octokit/request, but we can do that in a follow up PR

@wolfy1339
Copy link
Member Author

pika should be in another PR. Since it's not ready yet and depends on upstream to push a patch

@gr2m
Copy link
Contributor

gr2m commented May 1, 2019

As you prefer 👍

I don’t mind adding the patch as I did in @octokit/endpoint.js until upstream is resolved

@gr2m
Copy link
Contributor

gr2m commented May 7, 2019

pika should be in another PR. Since it's not ready yet and depends on upstream to push a patch

It’s been fixed upstream, you can copy the setup of endpoint.js.

I also finished the generated types via octokit/endpoint.js#53, I want to add that to @octokit/request as well, but we can do a follow up PR for that. Maybe we can reuse the options types

@wolfy1339
Copy link
Member Author

I'm going to mark this as ready for review, so that CI can run and merge conflicts appear

@wolfy1339 wolfy1339 marked this pull request as ready for review May 14, 2019 03:12
@wolfy1339
Copy link
Member Author

@gr2m I'm going to need your help with the right types and all. Tests are all broken because of this :/

@gr2m
Copy link
Contributor

gr2m commented May 14, 2019

I’m looking into it :)

@gr2m
Copy link
Contributor

gr2m commented May 14, 2019

Some of it got resolved by moving to @pika/pack. Do you mind if I adapt the setup to what we now have with @octokit/endpoint?

@wolfy1339
Copy link
Member Author

wolfy1339 commented May 14, 2019 via email

@gr2m
Copy link
Contributor

gr2m commented May 15, 2019

@wolfy1339 I think this is good to go? I’ve added a few TODOs to the pull request description, but we can make follow up issues for these and iterate

@gr2m gr2m changed the title [WIP] Convert project to typescript Convert project to typescript May 15, 2019
@wolfy1339
Copy link
Member Author

Looks fine and dandy

@gr2m
Copy link
Contributor

gr2m commented May 15, 2019

@octokit/rest is currently using internal APIs
https://github.com/octokit/rest.js/blob/7601bd78c449bf33c2058ce94e2ef3e7d3106c9c/lib/request-with-defaults.js#L3

I can think of two ways to address this

  1. Export fetchWrapper, which is what lib/request used to export

  2. Move the requestWithHook logic into @octokit/request, maybe as a secondary export?

     import { requestWithHook } from '@octokit/request'

The 1st option is simpler to implement and would be the same as things are today. I would not document it and continue to consider it to be usage of internal APIs which might break at any update.

The 2nd option is more elegant and would have the added benefit that we would no longer need to pin @octokit/request version 🤔

I’ll see how much of an effort the 2nd option will be, if I can’t figure it out tonight I’ll go with the 1st option and make a follow up issue

@gr2m
Copy link
Contributor

gr2m commented May 15, 2019

Adds a few bytes, but also lots of interesting possibilities

image

Any thoughts? I’ll sleep over it

gr2m added 26 commits May 15, 2019 14:26
BREAKING CHANGE: `const request = require("@octokit/request")` is now `const { request } = require("@octokit/rest")` or `import { request } from "@octokit/request"`
This has lots of duplication to @octokit/endpoint types, we can handle that later
I added a TODO for the pull request
Copy link
Contributor

@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.


bring it.

@gr2m gr2m merged commit 7b38319 into octokit:master May 15, 2019
@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.

3 participants