-
Notifications
You must be signed in to change notification settings - Fork 35
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
Ts migration #31
Ts migration #31
Conversation
@gr2m I think this is everything.
|
fyi I’ve put together some information on how I setup the other packages with I hope to get to review it in detail later today! |
c7d5977
to
a4f3b96
Compare
Ah shucks. Bundlesize is gone way up past the 1KB limit |
oh nice you got all the other tests passing? That’s great! Don’t worry about bundle size too much, it’s just a safety net to not accidentally increase the bundle size 10×. Just bump it to 1.5KB or 2KB. Maybe we’ll focus on bundle size some day, but that’s not our priority right now |
Thing is, it's closer to 15kb. Not single digits anymore, but double digits
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fantastic work!!! Just two minor comments!
@@ -0,0 +1,197 @@ | |||
import { install } from "lolex"; | |||
import nock from "nock"; | |||
import { stub } from "simple-mock"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think jest
has it’s own stubbing, maybe we use that instead of simple-mock?
https://jestjs.io/docs/en/mock-function-api.html
We can also do it in a follow up PR
Everything is done now. I've bumped up the bindlesize limit to 15kb, to make tests pass. |
BREAKING CHANGE: `const App = require("@octokit/app")` is now `const { App } = require("@octokit/app")` or `import { App } from "@octokit/app"`
I've rebased this and fixed all conflicts. It's good to go now |
Thanks @wolfy1339, I’m reviewing it now! Can we chat some time? I’ve pinged you in the Probot slack :) |
If I comment out the |
package.json
Outdated
"@pika/pack": "^0.3.7", | ||
"@pika/plugin-build-node": "^0.3.16", | ||
"@pika/plugin-build-node": "^0.4.0", | ||
"@pika/plugin-build-web": "^0.3.16", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missed @pika/plugin-build-web
?
Can we just remove it? It’s internal only anyway, isn’t it? |
I see, it's part of a bigger error. Yes, it's internal, we could remove it. |
I also don’t know but if removing is what it takes to unblock this PR, I’d just remove it? Or do you have something else in mind? |
Thing is, I can't remove it from the source files or else the compiler will complain that the property is not declared. It needs to be changed in the declaration files after the build
…________________________________
From: Gregor Martynus <notifications@github.com>
Sent: Tuesday, May 21, 2019 3:44:31 PM
To: octokit/app.js
Cc: wolfy1339; Mention
Subject: Re: [octokit/app.js] Ts migration (#31)
I also don’t know but if removing is what it takes to unblock this PR, I’d just remove it? Or do you have something else in mind?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub<#31?email_source=notifications&email_token=ABDB6FP657SH5M5QAEHWXBDPWRGJ7A5CNFSM4HNINWH2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODV47BQQ#issuecomment-494530754>, or mute the thread<https://github.com/notifications/unsubscribe-auth/ABDB6FMGVLJW3PQVUN73T4DPWRGJ7ANCNFSM4HNINWHQ>.
|
Okay I’ve an idea how to workaround this, I’ll try it later, but have to do some chores first |
Hmm no luck. First I tried sth like this function createApi({ id, privateKey, baseUrl, cache }: OctokitAppOptions) {
const state = {
id,
privateKey,
request: baseUrl ? request.defaults({ baseUrl }) : request,
cache: cache || getCache()
}
return {
getSignedJsonWebToken: getSignedJsonWebToken.bind(null, state),
getInstallationAccessToken: getInstallationAccessToken.bind(null, state)
}
}
export class App {
constructor(options: OctokitAppOptions): Api {
return createApi(options)
}
} But Typescript does not allow type annotations on constructors. And when trying export function App({ id, privateKey, baseUrl, cache }: OctokitAppOptions) {
const state = {
id,
privateKey,
request: baseUrl ? request.defaults({ baseUrl }) : request,
cache: cache || getCache()
};
return {
getSignedJsonWebToken: getSignedJsonWebToken.bind(null, state),
getInstallationAccessToken: getInstallationAccessToken.bind(null, state)
};
} Then the What do you think about this? export class App {
constructor({ id, privateKey, baseUrl, cache }: AppOptions) {
const state: State = {
id,
privateKey,
request: baseUrl ? request.defaults({ baseUrl }) : request,
cache: cache || getCache()
};
this.getSignedJsonWebToken = getSignedJsonWebToken.bind(null, state),
this.getInstallationAccessToken = getInstallationAccessToken.bind(null, state)
}
getSignedJsonWebToken: () => string
getInstallationAccessToken: (options: {
installationId: number;
}) => Promise<string>
} 🤔 |
Seems good. It's not exactly the same declarations outputted, but it's the same usage, and it works |
Okay thanks, I’ll finish it up then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉 This PR is included in version 3.0.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
No description provided.