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.

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

Fix "inngest/hono" serve handler not parsing environment variables #571

Merged
merged 8 commits into from
May 20, 2024

Conversation

jpwilliams
Copy link
Member

@jpwilliams jpwilliams commented May 15, 2024

Summary

The "inngest/hono" serve handler was not fetching environment variables, which are not reliably accessible via globals in common environments such as Cloudflare Workers.

In addition, url handling was a little wonky and didn't seem to handle many variants. The hope was that Hono handled that to ensure that c.req.url is always/never absolute, but this doesn't seem to be the case.

Checklist

  • Added a docs PR that references this PR N/A
  • Added unit/integration tests N/A
  • Added changesets if applicable

Related

@jpwilliams jpwilliams self-assigned this May 15, 2024
Copy link

changeset-bot bot commented May 15, 2024

🦋 Changeset detected

Latest commit: f1d8ad8

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
inngest Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@inngest-release-bot inngest-release-bot added the 📦 inngest Affects the `inngest` package label May 15, 2024
queryString: (key) => c.req.query(key),
headers: (key) => c.req.header(key),
method: () => c.req.method,
body: () => c.req.json(),
env: () => c.env as Env,
Copy link
Member Author

Choose a reason for hiding this comment

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

This should stop folks needing to manually specifying signing/event keys.

@@ -4,7 +4,8 @@
"deploy": "wrangler deploy --minify src/index.ts"
},
"dependencies": {
"hono": "^4.2.7"
"hono": "^4.2.7",
"inngest": "^3.0.0"
Copy link
Member Author

Choose a reason for hiding this comment

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

The example didn't include inngest 🫠

@jpwilliams jpwilliams added the prerelease/inngest Create snapshot releases for a PR for the "inngest" package. label May 15, 2024
@inngest-release-bot
Copy link
Collaborator

inngest-release-bot commented May 15, 2024

A user has added the prerelease/inngest label, so this PR will be published to npm with the tag pr-571. It will be updated with the latest changes as you push commits to this PR.

You can install this prerelease version with:

npm install inngest@pr-571

The last release was built and published from f1d8ad8.

@@ -35,11 +36,52 @@ export const serve = (options: ServeHandlerOptions) => {
transformResponse: ({ headers, status, body }) => {
return c.body(body, { headers, status });
},
url: () => new URL(c.req.url, c.req.header("host")),
url: () => {

Choose a reason for hiding this comment

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

Would canParse help here? it's safe to use becasue anyone using Hono would be using a runtime that supports it. https://developer.mozilla.org/en-US/docs/Web/API/URL/canParse_static

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, awesome. Aye, at a glance it looks like Hono supports 18+. Thanks!

jpwilliams and others added 5 commits May 15, 2024 17:26
This reverts commit 7455268.

Hono only supports environments that support `URL.canParse()`, but I'd
rather not adjust our runtime tests per handler, so am keeping this
verbose for Node 14-16 support.
@jpwilliams jpwilliams merged commit 67ca3aa into main May 20, 2024
39 checks passed
@jpwilliams jpwilliams deleted the fix/hono-cf branch May 20, 2024 12:12
jpwilliams pushed a commit that referenced this pull request May 20, 2024
This PR was opened by the [Changesets
release](https://github.com/changesets/action) GitHub action. When
you're ready to do a release, you can merge this and the packages will
be published to npm automatically. If you're not ready to do a release
yet, that's fine, whenever you add more changesets to main, this PR will
be updated.


# Releases
## inngest@3.19.4

### Patch Changes

- [#571](#571)
[`67ca3aa`](67ca3aa)
Thanks [@jpwilliams](https://github.com/jpwilliams)! - Fix
`"inngest/hono"` serve handler not handling relative and absolute
`req.url`

- [#571](#571)
[`67ca3aa`](67ca3aa)
Thanks [@jpwilliams](https://github.com/jpwilliams)! - Fix
`"inngest/hono"` serve handler not parsing environment variables

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📦 inngest Affects the `inngest` package prerelease/inngest Create snapshot releases for a PR for the "inngest" package.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Hono Adapter not working on Cloudflare Worker
3 participants