Ts migration

[wolfy1339]

No description provided.

[wolfy1339]

@gr2m I think this is everything.
Only a couple things remain:

• The custom cache test doesn't work anymore
• There is a timeout from mocha that i'm not exactly sure what to do/ how it works in jest

[gr2m]

fyi I’ve put together some information on how I setup the other packages with @pika/pack:
https://gist.github.com/gr2m/5dd2ec6e3a04c3dc79f0cbb46decbe46

I hope to get to review it in detail later today!

[wolfy1339]

Ah shucks. Bundlesize is gone way up past the 1KB limit

[gr2m]

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

[wolfy1339]

Thing is, it's closer to 15kb. Not single digits anymore, but double digits

[gr2m]

fantastic work!!! Just two minor comments!

[gr2m]

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

[wolfy1339]

Everything is done now. I've bumped up the bindlesize limit to 15kb, to make tests pass.

[wolfy1339]

I've rebased this and fixed all conflicts. It's good to go now

[gr2m]

Thanks @wolfy1339, I’m reviewing it now!

Can we chat some time? I’ve pinged you in the Probot slack :)

[gr2m]

I’m testing this PR against Probot’s test suite. One error I see is

That is not a very helpful error message 😞 any idea what it could mean?

[gr2m]

If I comment out the private api; line, Probot’s tests pass

[gr2m]

missed @pika/plugin-build-web?

[wolfy1339]

I’m testing this PR against Probot’s test suite. One error I see is

That is not a very helpful error message 😞 any idea what it could mean?

I don't understand at all. It's seems like a problem with the compiler, if I add the any type to the definition file, it works fine

[gr2m]

I don't understand at all. It's seems like a problem with the compiler, if I add the any type to the definition file, it works fine

Can we just remove it? It’s internal only anyway, isn’t it?

[wolfy1339]

I see, it's part of a bigger error. Yes, it's internal, we could remove it.
I'm not sure why it picks up though, it's supposed to be private and the declaration file contains no type for it, it's treated as an implicit any

[gr2m]

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?

[wolfy1339]

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

[gr2m]

Okay I’ve an idea how to workaround this, I’ll try it later, but have to do some chores first

[gr2m]

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 new keyword is no longer allowed, as expected.

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>
}

🤔

[wolfy1339]

Seems good. It's not exactly the same declarations outputted, but it's the same usage, and it works

[gr2m]

Okay thanks, I’ll finish it up then

[gr2m]

[octokitbot]

🎉 This PR is included in version 3.0.0 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀