-
-
Notifications
You must be signed in to change notification settings - Fork 619
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(cloudflare-pages): Expose Cloudflare Pages type parameters #3065
fix(cloudflare-pages): Expose Cloudflare Pages type parameters #3065
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## next #3065 +/- ##
==========================================
+ Coverage 96.08% 96.14% +0.06%
==========================================
Files 142 142
Lines 14408 14407 -1
Branches 2513 2514 +1
==========================================
+ Hits 13844 13852 +8
+ Misses 564 555 -9 ☔ View full report in Codecov by Sentry. |
function createEventContext( | ||
context: Partial<EventContext<Env['Bindings']>> | ||
): EventContext<Env['Bindings']> { | ||
return { | ||
data: {}, | ||
env: { | ||
...context.env, | ||
ASSETS: { fetch: vi.fn(), ...context.env?.ASSETS }, | ||
TOKEN: context.env?.TOKEN ?? 'HONOISCOOL', | ||
}, | ||
functionPath: '_worker.js', | ||
next: vi.fn(), | ||
params: {}, | ||
passThroughOnException: vi.fn(), | ||
request: new Request('http://localhost/api/foo'), | ||
waitUntil: vi.fn(), | ||
...context, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Creating an EventContext
in each test was getting quite repetitive, so I added this function that turns a Partial<EventContext>
into an EventContext
with all the necessary properties.
const reqInEventContext = c.env.eventContext.request | ||
return c.json({ TOKEN: c.env.TOKEN, requestURL: reqInEventContext.url }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From my understanding, this was ensuring that the original eventContext
was added to c.env
I replaced this by adding an assertion that vi.spyOn(app, 'fetch')
was called with { ...env, eventContext }
expect(next).not.toHaveBeenCalled() | ||
}) | ||
}) | ||
|
||
describe('serveStatic()', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also added test coverage for serveStatic()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
The tests are cool! Thanks! |
This PR aims to increase the type safety of the Cloudflare Pages handlers by exposing the type parameters of the
handle
andhandleMiddleware
functions. These types are really only useful when writing tests