feat: support usage in cloudflare workers #27
Conversation
|
any thoughts on updating node versions here? 14 support ended a while ago, and msw requires 18+ now. |
BREAKING CHANGE: node 18 is now the minimum supported version
|
May need to understand why CI fails here with an tests are passing locally in node 18 and 20 |
| // Ignore errors thrown by `request.credentials` access, such as those in cloudflare workers | ||
| // We can't just check that `credentials` is in request, because it is in cloudflare workers test environment | ||
| // however access throws an error | ||
| if ( |
There was a problem hiding this comment.
I'd split this if into two as they handle 2 distinct use cases. What do you think?
There was a problem hiding this comment.
We only want to preform this omit check in the case that the credentials are accessible
We do still want to deal with cookies though if credentials aren't.
if ( isPropertyAccessible(request, 'credentials')) {
if (request.credentials === 'omit') {
return
}
}we could do this instead - but i'm not sure the extra nesting gives us much? but I don't have strong feelings.
There was a problem hiding this comment.
Why not:
// Ignore the cases when "request.credentials" is not implemented.
if (!isPropertyAccessible(request, 'credentials')) {
return
}
// Handle the scenario when "credentials" was set to "omit".
if (request.credentials === 'omit') {
return
}There was a problem hiding this comment.
@kettanaito that check is diifferent than the one implemented here.
We do want to allow interacting with cookies when credentials are not implemented. We only want to check for omit when they are implemented.
the logic here is
- if credentials can be accessed and they are omit, don't interact
- if credentials is NOT implemented, or credentials are omit, return early
In your change we would return early if credentials are not implemented OR if they are omit. Which would fail to modify cookies in the case credentials are not implemented, which is not what we want in this case (as far as I can tell from a quick skimming of the docs on cloudflare - it is fine to interact with cookies there, there's just not credential check)
There was a problem hiding this comment.
Oh, I see now. Apologies, took me a minute to wrap my head around this. Please, feel free to proceed with your latest suggestion, it looks great then.
Released: v1.1.0 🎉This has been released in v1.1.0! Make sure to always update to the latest version ( Predictable release automation by @ossjs/release. |
Added a new test, using the miniflare jest-environment as well as some try/catch statements around access of properties not implemented in miniflare/cloudflare to support usage in those environments.
It seems cookies are accessible there, so we should work correctly, avoiding property access that is undefined in those environments.
miniflarewill throw on access ofrequest.credentialsand as such it defines it, so we cannot safely check'credentials' in request, and instead I've opted to try/catch around code that accesses properties like thatrelated to mswjs/msw#1834