chore: keep the call context on the event view instead of cloning the request state - #16968
chore: keep the call context on the event view instead of cloning the request state#16968Nic-Polumeyv wants to merge 11 commits into
Conversation
|
Install the latest version of pnpm add https://pkg.svelte.dev/@sveltejs/kit/c/8ff8bc6518d72dc644a1176deb7302c17b685db0Open in |
|
727ebe8 to
57e9cc0
Compare
Nic-Polumeyv
changed the title
57e9cc0 to
d77baa8
Compare
1dd3e60 to
f131d52
Compare
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: QUIET Plan: Advanced Run ID: 📒 Files selected for processing (7)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
Included review availability: Your plan provides up to 10 included reviews per hour; 6 remain after this review. WalkthroughThe change replaces transient request-state flags with event context kinds. Merge Risk: ⚪ Minimal · up to This change moves transient request-context state onto derived event views while preserving nested context checks and applying context-specific request restrictions. No concrete merge-blocking behavior, security, or runtime risk remains. Comment |
be2f7f2 to
e65743d
Compare
elliott-with-the-longest-name-on-github
left a comment
elliott-with-the-longest-name-on-github
left a comment
There was a problem hiding this comment.
This looks overall good, but I'm slightly concerned about the fact that we're putting the query restrictions on a prototype object, because prototypes aren't preserved through spreads.
The restrictions are applied through this:
const derived = Object.assign(Object.create(QUERY_PROTOTYPE), event, ...);...but in merge_tracing we spread the event:
export function merge_tracing(event_like, current) {
return {
...event_like,
tracing: {
...event_like.tracing,
current
}
};
}Object spread copies the enumerable properties, but does not preserve the prototype. Therefore:
const query_event = derive_event(event, 'query');
const traced_event = merge_tracing(query_event, span);...produces a normal object, where inside(traced_event, 'query') === true, but the protections for traced_event.url, .params, and .route are gone. I don't think we're actually doing this right now (so it's probably a latent bug). I'm not sure what exactly the best solution for that is... 🤔
|
Solid catch. v3 has the same hole (not 100% sure if you were alluding to already knowing that). But... the non-enumerable getters this replaces are skipped by that spread too, and either way it gives I'm thinking of just routing Edit: Actually, rather than patching const poison = (property) => {
const fail = () => {
throw new Error(`Cannot access event.${property} in a query. Pass the value as an argument to the query instead`);
};
return new Proxy(Object.freeze({}), { get: fail, has: fail, ownKeys: fail, set: fail });
};
const POISON = { url: poison('url'), params: poison('params'), route: poison('route') };Every copy mechanism carries data properties, and return {
...event,
...overrides,
...(entering & KINDS.remote ? restrictions(event, flags) : null),
...(flags & KINDS.query ? POISON : null),
[CONTEXT]: flags
};That also drops |
|
Hmm, I'm not sure I love that the error is moving from const url = getRequestEvent();
if (something) {
url.pathname // throws
}which just means there are more opportunities for you to write code that throws sometimes but not all the time. As much as I don't like the additional complexity it brings, I think using |
|
Ohh ok. I underweighted that. Going with my first option then |
…event" This reverts commit 1f7cef2.
2 participants
RequestStatecarries five booleans that are not about the request.is_in_render,is_in_remote_query,is_in_remote_prerender,is_in_remote_form_or_commandandis_in_remote_functionsay what kind of code is on the stack, and the only way to flip one is to clone the whole state object and re-enter the store with the copy, at 11 sites and twice per query call. It only stays correct because every field mutated inside remote code lives understate.remote, which the spread shares by reference.Kit already builds a per-call view of the event for remote functions, so the kind of code running is a property of that view.
derive_event(event, kind)now builds every view, stamps the kinds on the stack as bit flags under a symbol, and applies what that kind may not do (setHeaders, cookie writes,urlin a query);is_in(event, 'query')reads them. The flags accumulate through nested views exactly as the spreads did, so behaviour is unchanged, andRequestStatekeeps only per-request data.The
url/params/routeguards used to be redefined on each query view, which V8 answers by dropping the object into dictionary mode. They now live on a prototype, so every view keeps a fast shape.