Skip to content

Initial implementation #8

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

Initial implementation #8

wants to merge 1 commit into from

Conversation

baoshan
Copy link

@baoshan baoshan commented Jun 13, 2022 •
edited by wolfy1339
Loading

This PR includes a rewrite of my previous draft implementation and an updated README.md.

Below snippet demonstrates the basic usage.

import { Octokit } from "https://cdn.skypack.dev/@octokit/octokit";
import { createOAuthUserClientAuth } from "https://cdn.jsdelivr.net/gh/baoshan/auth-oauth-user-client.js@v0.1.0/dist/index.min.js";

const octokit = new Octokit({
  authStrategy: createOAuthUserClientAuth,
  auth: {
    clientId: "client_id", // get client id from https://github.com/settings/apps
    clientType: "github-app", // "github-app" | "oauth-app"
    expirationEnabled: true, // true | false
  },
});

const auth = await octokit.auth();
if (!auth) await octokit.auth({ type: "signIn" });
else console.log(await octokit.rest.users.getAuthenticated());

Although targeting browsers, this module is written, tested, and bundled using Deno for its simplicity. The (bundled and gzipped) size is slightly less than 2kB.

I’m grateful if @gr2m or @wolfy1339 could give it a review. Thanks again for your help.

This was referenced Jun 13, 2022
Open
Open
@baoshan baoshan force-pushed the main branch 3 times, most recently from 58e5d5f to b28bdb4 Compare June 13, 2022 16:54
@wolfy1339 wolfy1339 added the feature New feature or request label Jun 13, 2022
wolfy1339
wolfy1339 suggested changes Jun 13, 2022
View reviewed changes
.gitignore
@@ -0,0 +1,2 @@
.vscode/
Copy link
Member

Choose a reason for hiding this comment

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

please make sure to add node_modules

Copy link
Author

@baoshan baoshan Jun 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

There will not be node_modules because node and npm are not involved: instead of install dependencies, just deno test or deno bundle.

Please let me know if this is acceptable.

index.ts Outdated
@@ -0,0 +1,449 @@
// ## Types
Copy link
Member

Choose a reason for hiding this comment

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

Please split this file up into logical chunks, following the other Octokit repositories format

Copy link
Member

Choose a reason for hiding this comment

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

Please use #1 as a base for this.

Copy link
Author

Choose a reason for hiding this comment

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

  • Modules are separated into files.
  • Coverage is 100%.
  • I believe Renovate / WIP / Octokit JS Board apps may be installed in PRs following this.

This strategy is intended to target browsers. The bundled JavaScript file can be simply made using deno bundle. If you think an npm package is still necessary, I will try to add a script to publish to npm.

@baoshan baoshan requested a review from wolfy1339 June 14, 2022 07:35
@gr2m
Copy link
Contributor

gr2m commented Jun 14, 2022

I'm afraid about the maintainability of this module, as it's setup is too different from the other modules. The @octokit team doesn't use Deno in our stack yet at all, it's something that might be worth revisiting once the work on https://github.com/octokit/octokit-next.js is complete, but there is a lot of work left on it.

@baoshan Maybe, for now, you can release your own package as octokit-auth-oauth-user-client so you can start using it?

@baoshan
Copy link
Author

baoshan commented Jun 15, 2022

Thanks for the suggestions. I’ve published the octokit-auth-oauth-user-client module.

Closing for now.

@baoshan baoshan closed this Jun 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wolfy1339 wolfy1339 Awaiting requested review from wolfy1339

Assignees
No one assigned
Labels
feature New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@baoshan @gr2m @wolfy1339