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.

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

feat: createWebWorkerHandler for Cloudflare and Deno. Deprecates createCloudflareHandler #287

Merged
merged 1 commit into from Jul 1, 2022
Merged

feat: createWebWorkerHandler for Cloudflare and Deno. Deprecates createCloudflareHandler #287

merged 1 commit into from Jul 1, 2022

Conversation

baoshan
Copy link
Contributor

@baoshan baoshan commented Jun 22, 2022

The current implementation of createCloudflareHandler is not generic, preventing it from accepting an OAuthApp with clientType: "github-app".

This PR also renames createCloudflareHandler to createWebWorkerHandler. The same handler supports both Cloudflare workers and Deno. Documentation is updated to reflect the change.

@ghost ghost added this to Inbox in JS Jun 22, 2022
@wolfy1339 wolfy1339 added the Type: Feature New feature or request label Jun 22, 2022
@ghost ghost moved this from Inbox to Features in JS Jun 22, 2022
@wolfy1339 wolfy1339 added the deno label Jun 22, 2022
@wolfy1339
Copy link
Member

You seem to have included a commit which is not needed, as it's changes are already present on the repository c632013 should be removed from the PR

wolfy1339
wolfy1339 reviewed Jun 22, 2022
View reviewed changes
package-lock.json Outdated Show resolved Hide resolved
wolfy1339
wolfy1339 previously approved these changes Jun 22, 2022
View reviewed changes
Copy link
Member

@wolfy1339 wolfy1339 left a comment

Choose a reason for hiding this comment

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

Looks good to me!
Just remove the package-lock.json to avoid any conflicts

wolfy1339
wolfy1339 previously approved these changes Jun 22, 2022
View reviewed changes
@gr2m gr2m changed the title Make createWebWorkerHandler generic to support both client types feat: createWebWorkerHandler for Cloudflare and Deno Jun 22, 2022
@baoshan baoshan mentioned this pull request Jun 22, 2022
Merged
@baoshan baoshan force-pushed the web_worker_handler branch 2 times, most recently from 3819a91 to d272b01 Compare June 22, 2022 18:53
@baoshan
Copy link
Contributor Author

baoshan commented Jun 22, 2022

I wish it is ready to be merged. Thanks @wolfy1339 and @gr2m for your help. ❤️

wolfy1339
wolfy1339 previously approved these changes Jun 22, 2022
View reviewed changes
gr2m
gr2m previously approved these changes Jun 22, 2022
View reviewed changes
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.

Thank you Baoshan!

gr2m
gr2m reviewed Jun 22, 2022
View reviewed changes
src/index.ts Outdated Show resolved Hide resolved
@baoshan baoshan dismissed stale reviews from gr2m and wolfy1339 via f680a93 June 23, 2022 01:37
@baoshan baoshan force-pushed the web_worker_handler branch 2 times, most recently from f680a93 to 6e1dc40 Compare June 23, 2022 01:44
gr2m
gr2m reviewed Jun 23, 2022
View reviewed changes
Copy link
Contributor

@gr2m gr2m left a comment •
edited

Choose a reason for hiding this comment

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

one last thing, promise! We should test that createCloudflareHandler is still working and that it logs out a deprecation message. I like to create a dedicated file to test deprecations: test/deprecations.test.ts. That way it's easy to remove all deprecations each time we release a breaking version.

Would you mind adding the file, testing that createCloudflareHandler still works as expected and that it logs the warning?

@baoshan
Copy link
Contributor Author

baoshan commented Jun 24, 2022

Thanks for the suggestion! Done. Feel free to let me know if there is still room to improve.

wolfy1339
wolfy1339 approved these changes Jun 30, 2022
View reviewed changes
Copy link
Member

@wolfy1339 wolfy1339 left a comment

Choose a reason for hiding this comment

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

Looks good

gr2m
gr2m approved these changes Jul 1, 2022
View reviewed changes
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.

great PR, thank you Baoshan!

@gr2m gr2m changed the title feat: createWebWorkerHandler for Cloudflare and Deno feat: createWebWorkerHandler for Cloudflare and Deno. Deprecates createCloudflareHandler Jul 1, 2022
@gr2m gr2m merged commit 31b3f70 into octokit:master Jul 1, 2022
JS automation moved this from Features to Done Jul 1, 2022
@github-actions
Copy link

github-actions bot commented Jul 1, 2022

🎉 This PR is included in version 3.7.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@github-actions
Copy link

github-actions bot commented Jul 8, 2022

🎉 This PR is included in version 3.7.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@baoshan baoshan deleted the web_worker_handler branch August 31, 2022 08:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gr2m gr2m gr2m approved these changes

@wolfy1339 wolfy1339 wolfy1339 approved these changes

Assignees
No one assigned
Labels
deno Type: Feature New feature or request
Projects
No open projects
JS
  
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@baoshan @wolfy1339 @gr2m