🚧 Initial version

[gr2m]

• [ ] Implement features. Create separate feat: ... commits for each feature of the initial version
• [ ] 100% test coverage
• [ ] Install the Renovate app
• [x] Install the WIP app
• [x] Install the Octokit JS Board app

[baoshan]

Any chance this initial version will be published and supported? Looking forward to it!

[gr2m]

I'm not working on it right now. I only ran the setup script npm init octokit-project and created a README: https://github.com/octokit/auth-oauth-user-client.js/tree/initial-version#readme

If you'd like to work on it yourself let me know

[baoshan]

Aha! Thanks for the information! Is this client (and the authentication flow) compatible with your Cloudflare Worker?

[gr2m]

It should be, if it's not I can help make it so. I'll have a look at https://github.com/gr2m/cloudflare-worker-github-oauth-login today to make sure it updated to the latest of everything. If you get stuck with anything please let me know :)

[gr2m]

so actually https://github.com/gr2m/cloudflare-worker-github-oauth-login is not a good backend for it.

What you should use for the backend is @octokit/oauth-app (or @octokit/app, which includes @octokit/oauth-app). In particular, see the routes defined here: https://github.com/octokit/oauth-app.js#middlewares. These are the routes that will map perfectly to the features I'd like to see implemented in @octokit/auth-oauth-user-client

It should be straight forward to deploy an OAuth app using @octokit/oauth-app to Glitch. If you like I can do that and invite you to the glitch app? We could also add a local dev server for local testing to this repository?

[gr2m]

there you go: https://glitch.com/edit/#!/github-oauth-app-example

[baoshan]

I’d like try Cloudflare Worker (the first time) for my next project.

I haven’t dive deep yet, but do you think it is possible to create a Cloudflare Worker which is compatible with @octokit/oauth-app (and also the to be implemented @octokit/auth-oauth-user-client.js as you envisioned)? If it is possible, I would like to contribute (both the worker and the client).

Thanks :)

[gr2m]

Yes it’s absolutely possible. You can use the createNodeMiddleware() implementation (source) as a blueprint. I'd love a middleware for Cloudflare Workers, when you build it we can link it from the repository's README

[baoshan]

Can the (to be implemented) Cloudflare middleware benefit from @octokit/oauth-methods and hence @octokit/request? Are these low level @octokit/* modules Cloudflare compatible? Or is it better to add the create/check/reset/refresh/delete routes to cloudflare-worker-github-oauth-login.js as how it handles the token creation currently?

I guess some of the code in @octokit/oauth-app.js can be reused in the Cloudflare middleware oauth-app-cloudflare.js. For the client side, I also think @octokit/auth-oauth-user-client.js should share some code with @octokit/auth-oauth-user.js (types, etc.).

I believe cloudflare-worker-github-oauth-login.js does fit my needs for now although it is unable to be integrated with octokit.js in the browser. Oh my fault, it definitely can be used with octokit.js: just pass the token returned from the worker as a static one to octokit.js. May I ask: what is the benefit of auth-oauth-user-client.js + oauth-app-cloudflare.js over cloudflare-worker-github-oauth-login.js + octokit.js?

Thanks again for your patience.

[gr2m]

Can the (to be implemented) Cloudflare middleware benefit from @octokit/oauth-methods and hence @octokit/request? Are these low level @octokit/* modules Cloudflare compatible?

Yes, they are! @octokit/request is depending on node-fetch, but for Cloudflare it should skip that dependency, as Cloudflare has the native fetch.

Or is it better to add the create/check/reset/refresh/delete routes to cloudflare-worker-github-oauth-login.js as how it handles the token creation currently?

I built https://github.com/gr2m/cloudflare-worker-github-oauth-login before @octokit/oauth-methods, otherwise I'd have used that

I guess some of the code in @octokit/oauth-app.js can be reused in the Cloudflare middleware oauth-app-cloudflare.js.

Yes, you should be able to use it. But I think Cloudflare workers work like serverless functions in that they don't keep state between requests? I'm not 100% sure about that. If they keep state, I'd use @octokit/auth-app. If they don't, I'd use the stateless @octokit/oauth-methods

For the client side, I also think @octokit/auth-oauth-user-client should share some code with @octokit/auth-oauth-user (types, etc.).

Both the code and the types of @octokit/auth-oauth-user.js is built on the assumption that clientSecret is set. That will not be the case for @octokit/auth-oauth-user-client. I'm sure their will be a lot of overlap, but for the sake of simplicity, I'd suggest to just copy & paste for now to make it work, and then we can look into how we can refactor it later, in order to remove code duplication?

what is the benefit of auth-oauth-user-client.js + oauth-app-cloudflare.js over cloudflare-worker-github-oauth-login.js + octokit.js?

My longer term goal is to server both a pre-configured @octokit/auth-oauth-user-client and a pre-authenticated/configured Octokit constructor from the OAuth server itself.

Right now, @octokit/oauth-app exposes all the {GET,POST,PATCH,DELETE} /api/github/* routes. Now imagine there would an cloudflare-octokit-app package that you'd only need to configure and deploy as Cloudflare worker, and it would not only expose the current {GET,POST,PATCH,DELETE} /api/github/* routes, but also GET /api/github/auth.js and GET /api/github/octokit.js.

GET /api/github/auth.js would give you a createOAuth function that you could just call without any configuration:

import { createOAuth } from "/api/github/auth.js"
const auth = createOAuth()

It would work just like the usage example but all configuration is preset for you, as it's tailored to your server instance.

And even simpler, you can load the Octokit constructor which is loading createOAuth and has authStrategy: createOAuth preset, so you can just do

import { Octokit } from "/api/github/auth.js"
const octokit = Octokit()

and it would just work™️

To circle back about the benefit: expiring OAuth tokens will become the norm in future. They are already enabled for new GitHub Apps by default. After 8h a token expires and is no longer valid. @octokit/auth-oauth-user-client can take of the renewal for you. It would automatically store both the authentication token and the reset token in browser storage (or just memory, depending on how you configure it for your app) and it would automatically renew a token if the server responds with a 401 token expired error.

[baoshan]

Hi, Gregor! It's been a long week. Do you have some time to review the Cloudflare worker?

I’ve drafted two commits:

1. I made a runtime agnostic handler from the original middleware. The new handler operates on a generalized request/response interface. All parameters are immutable. A general response is returned (or null if not handled). An individual runtime should provide its own parse-request and send-response methods, which is usually thin (okay, for Node.js HTTP and Fetch Request/Response at least). The original test is untouched so I guess it is does not change the behavior. I also removed "fromEntries" dependency since Node.js v10 is not maintained now.

2. A Cloudflare Workers version is created based on the first commit. Both Cloudflare and Node.js version rely on the same handler and have the same behavior. I used the new "modules" Cloudflare type so package.json does not need to have a "main" entry which is unacceptable.

Do you think it makes sense? Do I need to look at @octokit/types and be consistent with something?

For auth-oauth-user-client.js, I guess the getSession/createToken/... options in your initial README are all optional and the default implementations are to be provided by the library to aiming all-batteries-included. Am I correct? I see two versions: the standalone version uses Fetch APIs and the other uses on @octokit/request (and the hook mechanism in core to provide authorization header I guess). Can I provide a single suite of default getSession/createToken/... functions (using Fetch APIs) and another hook method for other requests issued directly by core? Are these default functions (I can copy & paste the README 😄) and the hook what I need to implement for auth-oauth-user-client.js?

Sorry for being lazy.

[gr2m]

wow, thanks a lot for taking it on Baoshan!

1. I made a runtime agnostic handler from the original middleware. The new handler operates on a generalized request/response interface

I need to find a bigger chunk of time to review your changes. Could you start a pull request? That will make it easier to discuss the changes

2. A Cloudflare Workers version is created based on the first commit

Can you start a pull request as well, maybe against the branch of the first pull request, so it would only show the relevant changes?

Do I need to look at @octokit/types and be consistent with something?

I do not think so, the types in there are more for the Octokit API client SDK, not for any server implementations.

For auth-oauth-user-client.js, I guess the getSession/createToken/... options in your initial README are all optional and the default implementations are to be provided by the library to aiming all-batteries-included. Am I correct?

Yes. I'd say the default implementation should be something like

function getSession () {
  throw new Error("[@octokit/auth-oauth-user-client] getSession implementation is not set")
}

> I see two versions: the standalone version uses Fetch APIs and the other uses on `@octokit/request` (and the hook mechanism in `core` to provide authorization header I guess)

The way the other authentication strategies are implemented is to use [`@octokit/request`](https://github.com/octokit/request.js) (which is a slight wrapper around `fetch`). The default `request` from `@octokit/requst` can be overwritten by a strategy option `request`, which is how an `Octokit` constructor can hook into the requests done by an authentication strategy. See for example the [strategy options for `@octokit/auth-oauth-client`](https://github.com/octokit/auth-oauth-user.js#createoauthuserauthoptions-or-new-octokit-auth-). Passing `octokit.request` as the `request` option to the authentication strategy set as `authStrategy` option on the `Octokit` constructor is implemented in `@octokit/core`: 
https://github.com/octokit/core.js/blob/5e85e828d01e95eaffd82016dc580c5ae065c305/src/index.ts#L141-L156

That's a mouth full 🤣 What it means when you do this

```js
const octokit = new Octokit({
  authStrategy: createOAuthUserClientAuth,
  auth: { getSession, /* ... /* }
})

Then createOAuthUserClientAuth will be called with the options from auth plus { log, request, octokit, octokitOptions }.

So in the createOAuthUserClientAuth function, I'd implement a request option that defaults to @octokit/request.

@octokit/request is 3.6kb: https://bundlephobia.com/package/@octokit/request, I think that's ok given its simpler API.

[gr2m]

Thanks @baoshan, I'll need a moment to review the pull request.

One thing I've seen is that you've used conditional chaining (something?.something). We can't use it as we still support Node 12. We use a es2020 in tsconfig because otherwise the generated code is blown up a lot by poly-fills we don't need. The downside is that the compiler does not complain about using conditional chaining and it only fails in CI.

[gr2m]

Given that this library is not meant to be used in Node we can also remove the tests in Node 12 altogether. We should probably add tests using real browsers as part of the CI, but I'd say we can look into that later

[baoshan]

Thanks for the suggestions! I’m ready to help when you’ve reviewed the PR.

[gr2m]

@baoshan hey just checking in to make sure you are not blocked by something? Anything I can help with?

[baoshan]

Are you expecting me to fix the conditional chaining issue?

Is it fine to use jest for now? Is there anything else that doesn’t look good? I thought there may be other issues in the commit to discuss so I didn’t fix that (conditional chaining) issue in time.

Sorry for the delay anyway, I’ll submit a PR later.

[gr2m]

Are you expecting me to fix the conditional chaining issue?

no need, as it's a new package and we don't need to support Node 10.

Is it fine to use jest for now? Is there anything else that doesn’t look good? I thought there may be other issues in the commit to discuss so I didn’t fix that (conditional chaining) issue in time.

Yes Jest is fine

Sorry for the delay anyway, I’ll submit a PR later.

No worries at all, no rush.

I just realized I meant to do a thorough review of this PR, I'll do that tomorrow

[ArtemMelnychenko]

@gr2m Hello. Do you need any help on this? I have managed to fix test errors on my fork (by removing ?. and ?? and syntax), and also there was a bug in the path to token (this.session was set to response.data instead of {authentication: response.data}).

I have tested it locally with the POC project I was working on, it works :)

[gr2m]

@ArtemMelnychenko Any help is much appreciated! Can you send a PR against the initial-version branch?

I'm focused on https://github.com/octokit/octokit-next.js right now and for the coming weeks and months, but I'd love to get @octokit/auth-oauth-user-client released

[baoshan]

there was a bug in the path to token (this.session was set to response.data instead of {authentication: response.data}).

Hi @ArtemMelnychenko! Thanks for testing the draft!

I’m not sure why you need to set this.session to {authentication: response.data}. oauth-app.js middleware responds with {authentication: {...}} already (see test).

Could you let me know why the current implementation fails on your POC project? Thanks.

[baoshan]

Personally I hope the nullish coalescing operator (??) and optional chaining (?.) are fine for the source code. They’re supported by major browsers for more than 18 months. If we need to cover older browsers, we can change tsconfig.json to output es2019 code.

[Plazmius]

@baoshan thanks for the clarification. Indeed, it's covered by a test and there was a link to the oauth-app.js default handler for this in the initial-version branch readme . I think I have skipped through because I was looking at the main branch readme and didn't find the link. So I have implemented the handler myself, sorry for the misleading commit.

regarding coalescing operator (??) and optional chaining (?.) - I have tested inside docker node:12 image and can confirm that setting the target to es2019 allows jest tests to succeed. However, pika build throws a warning tsconfig.json [compilerOptions.target] should be "ES2020", but found "ES2019". You may encounter problems building. and compiles to ES2020 anyway.`

Should I proceed with reverting the previous commits and add es2019 target in my pull request?

Edit: alternatively, we could override target in the global jest config as described here. I have tested it too, then we can avoid the warning.

[baoshan]

Thanks very much @Plazmius for letting me know the details.

I’m fine with removing node 12 from the matrix. I guess esbuild is more suitable for front-end bundling if old browsers should be supported since @pika/plugin-standard-pkg always compiles to es2020.

I think @gr2m is too busy to review the code. If that is the case, could you kindly help him by giving initial-version branch a review? If everything looks good, could @gr2m publish a release? I will keep watching the project for issues reported.

BTW, will this be compatible with octokit-next?

[Plazmius]

@baoshan I will submit the review with pleasure. Should I create a PR for removing test matrix for node 12 first, or go ahead with the review? Is there any guidelines/requirements for submitting the review, because I didn't find any? Thanks

[gr2m]

I’m fine with removing node 12 from the matrix. I guess esbuild is more suitable for front-end bundling if old browsers should be supported since @pika/plugin-standard-pkg always compiles to es2020.

I'm okay with that, too. I'd rather unblock us here and change things later :)

I think @gr2m is too busy to review the code.

Sorry for the delay, I'm trying to catch up soon

BTW, will this be compatible with octokit-next?

Don't worry about that right now, I'll do once we get there. I cannot think of a reason why it shouldn't

[baoshan]

There’re several known issues in my draft implementation (the initial-version branch of this repo currently). I’m willing to submit a PR if maintainers decided to move on. Just let me know.

For anybody who needs this strategy now, you can use my fork.

[gr2m]

@baoshan hey I'm interested in moving this forward :) What's the latest status with your port? We are in the process of removing the pika build tools and replacing them with esbuild and tsc (see for example https://github.com/octokit/app.js/pull/420).

I'd also be interested in figuring out how to build a React Provider for Octokit using this plugin. Do you know if there is any prior art in that direction?

[baoshan]

It has no known issue but I may be the only user.

• I use a single-file layout.
• I use deno bundle mod.ts index.bundle.js to bundle for the browser.

I do not currently have time to rewrite the tests in Jest (which is heavy IMHO) or make an esbuild script.

PS: I know little about React.

[gr2m]

Sounds good, I'll see if I can find the time this weekend to merge your changes and adapt them to the @octokit conventions

Jest (which is heavy IMHO)

yeah I replaced Jest with ava in https://github.com/octokit/octokit-next.js for that reason.