chore: sync main branch with store refactor#7001
chore: sync main branch with store refactor#7001birkskyum merged 32 commits intosolid-router-v2-prefrom
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
bot
added
documentation
|
View your CI Pipeline Execution ↗ for commit 9b63607
☁️ Nx Cloud last updated this comment at |
🚀 Changeset Version Preview6 package(s) bumped directly, 0 bumped as dependents. 🟩 Patch bumps
|
Bundle Size Benchmarks
Trend sparkline is historical gzip bytes ending with this PR measurement; lower is better. |
Merging this PR will improve performance by 55.79%
|
| Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|
| ⚡ | ssr request loop (vue) |
514 ms | 417.3 ms | +23.18% |
| ⚡ | ssr request loop (solid) |
226 ms | 179.1 ms | +26.2% |
| ⚡ | client-side navigation loop (solid) |
64.2 ms | 42.6 ms | +50.68% |
| ⚡ | client-side navigation loop (react) |
69.8 ms | 44.8 ms | +55.79% |
| ⚡ | client-side navigation loop (vue) |
75.1 ms | 58.6 ms | +28.08% |
Comparing solid-router-main-merge (9b63607) with solid-router-v2-pre (cde6e61)1
Footnotes
| batch: (fn) => { | ||
| fn() | ||
| // In Solid v2, signal updates are batched by default and only | ||
| // committed when the reactive system flushes. When batch() is | ||
| // called from outside a reactive context (e.g. from router.load() | ||
| // invoked directly in test code or before render), we must flush | ||
| // manually so that derived stores (memos) see the new values | ||
| // synchronously. Inside a reactive scheduling context (effects, | ||
| // onSettled, createTrackedEffect), flush() is not reentrant and | ||
| // will throw, so we catch and ignore that case since Solid will | ||
| // flush automatically when the effect completes. | ||
| try { | ||
| Solid.flush() | ||
| } catch { | ||
| // flush() throws when called from inside onSettled or | ||
| // createTrackedEffect — Solid will flush automatically there. | ||
| } | ||
| }, |
There was a problem hiding this comment.
Should we track depth to avoid flushing too much?
let depth = 0
batch: (fn) => {
depth++
fn()
depth--
if (depth === 0) {
try {
Solid.flush()
} catch {}
}
}Also, just so I understand: memos keep carrying the old value until we flush?
Like if we do this:
const [foo, setFoo] = createSignal(1)
const double = createMemo(() => foo() * 2)
setFoo(2)
console.log(double())This logs 2 and not 4?
If this is the case, then we might have other issues in router-core. I've tried using batch() every time we are setting multiple stores in the same tick, but I've never used it for a single setter. There might be places in router-core where we try to read a derived value just after setting it synchronously.
There was a problem hiding this comment.
yeah that is my understanding
There was a problem hiding this comment.
Ok added the depth check now
|
Should we worry about the bundle size increase (router +3.93 KiB, start +9.53 KiB)? Or is it expected moving to solid v2? (BTW, bundle-size deltas are always compared against |
|
Im a little concerned about bundle size. I expect a little bump from solid 2 but this is a decent amount |
* fix: better react HMR * layout and fixes * fix * stabilize test * stabilize test
birkskyum
changed the title
6 participants
No description provided.