Conversation
🦋 Changeset detectedLatest commit: 8b9b0c6 The changes in this PR will be included in the next version bump. 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 |
There was a problem hiding this comment.
Pull request overview
Updates Qwik core async-signal loading publication behavior and extends the optimizer’s root-variable migration to preserve declaration kinds while adding a new diagnostic for unsafe shared mutable root bindings across segments.
Changes:
- AsyncSignal: only publish
loading=trueto subscribers when computation is actually async (QRL resolve or promise-returning compute). - Optimizer: preserve original
let/var/constwhen migrating module-scope vars into segment modules; introduce diagnosticC06 SharedMutableRootVar. - Add optimizer tests + snapshots covering (a) error on shared module-scope
letused by both root + segment, and (b) preservingletwhen migrated into a segment.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/qwik/src/core/reactive-primitives/impl/async-signal-impl.ts | Adjusts when loading state is published to avoid transient loading states for sync resolves. |
| packages/optimizer/core/src/parse.rs | Emits new shared-mutable-root-var error and preserves original var decl kind when migrating. |
| packages/optimizer/core/src/dependency_analysis.rs | Tracks original var declaration kind (let/var/const) in dependency metadata. |
| packages/optimizer/core/src/errors.rs | Adds new diagnostic rule/code mapping (SharedMutableRootVar → C06). |
| packages/optimizer/core/src/test.rs | Adds new regression tests for shared mutable root let and let-preservation on migration. |
| packages/optimizer/core/src/snapshots/* | Adds snapshots for the new optimizer tests. |
| .changeset/loose-lemons-read.md | Declares a minor optimizer release for the migration behavior change. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Emit errors for mutable module-scope bindings (`let` / `var`) that are referenced | ||
| // by a segment but cannot be migrated (shared with main module or other segments). | ||
| // Extracted segments live in separate ES modules; they would receive a read-only | ||
| // import binding and could not write back to the parent module's `let`/`var`. | ||
| { |
There was a problem hiding this comment.
This diagnostic currently triggers for any module-scope let/var referenced by a segment but not migrated. root_var_usage is built from local_idents/scoped_idents (reads as well as writes), so this will also error for read-only references where importing a let binding is valid. Consider tightening the condition to only error when the identifier is mutated inside a segment (e.g. update/assignment ops), or adjust the diagnostic/message to match the intended restriction.
| is_imported: false, | ||
| is_exported: user_exported.contains(&var_id), | ||
| depends_on: Vec::new(), | ||
| var_kind: Some(var_decl.kind), | ||
| }, |
There was a problem hiding this comment.
var_kind is only populated for ModuleItem::Stmt(Stmt::Decl(Decl::Var)) declarations. export let/var/const ... is represented as ModuleDecl::ExportDecl, so those bindings will end up with var_kind: None (and often a synthetic decl/span), which can make migrated declarations default to const and can also cause the new shared-mutable diagnostic to be skipped. Consider handling ExportDecl var declarations in the same way as non-exported ones (both for var_kind and for capturing the original decl/span).
| '@qwik.dev/optimizer': minor | ||
| --- | ||
|
|
||
| FEAT: module-scoped `let` identifiers retain `let` when migrating |
There was a problem hiding this comment.
This changeset describes the let-preservation behavior, but this PR also introduces a new optimizer error (C06 / SharedMutableRootVar) for shared mutable module-scope bindings captured by segments. Consider mentioning the new diagnostic/behavior change here as well so consumers understand the new failure mode when upgrading.
| FEAT: module-scoped `let` identifiers retain `let` when migrating | |
| FEAT: module-scoped `let` identifiers retain `let` when migrating. | |
| This change also introduces optimizer error `C06` / `SharedMutableRootVar` for | |
| shared mutable module-scope bindings captured by segments, so consumers may see | |
| a new failure mode when upgrading. |
built with Refined Cloudflare Pages Action⚡ Cloudflare Pages Deployment
|
@qwik.dev/core
@qwik.dev/router
eslint-plugin-qwik
create-qwik
@qwik.dev/optimizer
commit: |
Varixo
left a comment
Varixo
left a comment
There was a problem hiding this comment.
LGTM, but look at ai review for the optimizer
|
@copilot review |
Addressed the optimizer AI review item in commit
Completed a follow-up optimizer review and applied a fix in commit |
github-project-automation
Bot
moved this from Waiting For Review
to Done
in Qwik Development
let countertoconst counter