Skip to content

feat: Enhance optimizer to use _fnSignal for derived signal express…#8420

Open
JerryWu1234 wants to merge 3 commits intoQwikDev:build/v2from
JerryWu1234:4769_fix
Open

feat: Enhance optimizer to use _fnSignal for derived signal express…#8420
JerryWu1234 wants to merge 3 commits intoQwikDev:build/v2from
JerryWu1234:4769_fix

Conversation

@JerryWu1234
Copy link
Contributor

@JerryWu1234 JerryWu1234 commented Mar 12, 2026 •
edited
Loading

…ions and improve constant detection for safe global calls.

What is it?

  • Feature / enhancement
  • Bug
  • Docs / tests / types / typos
  • Infra

Description

#4769

Checklist

  • My code follows the developer guidelines of this project
  • I performed a self-review of my own code
  • I added a changeset with pnpm change
  • I made corresponding changes to the Qwik docs
  • I added new tests to cover the fix / functionality

@JerryWu1234 JerryWu1234 requested a review from a team as a code owner March 12, 2026 06:11
@changeset-bot
Copy link

changeset-bot bot commented Mar 12, 2026 •
edited
Loading

🦋 Changeset detected

Latest commit: 61eb8b9

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

@pkg-pr-new
Copy link

pkg-pr-new bot commented Mar 12, 2026 •
edited
Loading

Open in StackBlitz

npm i https://pkg.pr.new/QwikDev/qwik/@qwik.dev/core@8420

npm i https://pkg.pr.new/QwikDev/qwik/@qwik.dev/router@8420

npm i https://pkg.pr.new/QwikDev/qwik/eslint-plugin-qwik@8420

npm i https://pkg.pr.new/QwikDev/qwik/create-qwik@8420

commit: 61eb8b9

@github-actions
Copy link
Contributor

github-actions bot commented Mar 12, 2026 •
edited
Loading

built with Refined Cloudflare Pages Action

⚡ Cloudflare Pages Deployment

Name Status Preview Last Commit
qwik-docs ✅ Ready (View Log) Visit Preview 61eb8b9

wmertens
wmertens requested changes Mar 12, 2026
View reviewed changes
...qwik/src/optimizer/core/src/snapshots/qwik_core__test__example_derived_signals_children.snap
/*#__PURE__*/ _jsxSorted("div", null, null, globalThing.thing, 1, null),
/*#__PURE__*/ _jsxSorted("div", null, null, globalThing.thing + 'stuff', 1, null),
/*#__PURE__*/ _jsxSorted("div", null, null, globalThing, 3, null),
/*#__PURE__*/ _jsxSorted("div", null, null, _wrapProp(globalThing, "thing"), 3, null),
Copy link
Member

@wmertens wmertens Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

...qwik/src/optimizer/core/src/snapshots/qwik_core__test__example_derived_signals_children.snap
Comment on lines 102 to 104
/*#__PURE__*/ _jsxSorted("div", null, null, dep, 3, null),
/*#__PURE__*/ _jsxSorted("div", null, null, dep.thing, 3, null),
/*#__PURE__*/ _jsxSorted("div", null, null, dep.thing + 'stuff', 3, null),
Copy link
Member

@wmertens wmertens Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think these can all be made into _fnSignal

...qwik/src/optimizer/core/src/snapshots/qwik_core__test__example_derived_signals_children.snap
Comment on lines +107 to 111
/*#__PURE__*/ _jsxSorted("div", null, null, globalThing.thing + 'stuff', 3, null),
/*#__PURE__*/ _jsxSorted("div", null, null, signal.value(), 1, null),
/*#__PURE__*/ _jsxSorted("div", null, null, signal.value + unknown(), 1, null),
/*#__PURE__*/ _jsxSorted("div", null, null, signal.value + unknown(), 3, null),
/*#__PURE__*/ _jsxSorted("div", null, null, mutable(signal), 1, null),
/*#__PURE__*/ _jsxSorted("div", null, null, signal.value + dep, 1, null)
Copy link
Member

@wmertens wmertens Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these can also be turned into _fnSignal I think

Copy link
Contributor Author

@JerryWu1234 JerryWu1234 Mar 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried some approaches, but it failed ..

@wmertens
Copy link
Member

wmertens commented Mar 12, 2026

I'm not sure what breaks if we just wrap all expressions into fnSignal, but I can't think of anything 🤔
@Varixo ?

@Varixo
Copy link
Member

Varixo commented Mar 12, 2026

I'm not sure what breaks if we just wrap all expressions into fnSignal, but I can't think of anything 🤔 @Varixo ?

We shouldnt wrap everything, because it means more vnode data and state to serialize, but idk if it breaks something

@wmertens
Copy link
Member

wmertens commented Mar 13, 2026

@Varixo so we should wrap only things that aren't referenced in the component, right?

For example, if a component uses foo.bar in the render function and in the jsx it also uses foo.bar, then we don't need to wrap

but if it uses untrack(() => foo.bar) then we should wrap

@wmertens wmertens added the V2 label Mar 15, 2026
@JerryWu1234 JerryWu1234 self-assigned this Mar 17, 2026
@JerryWu1234 JerryWu1234 force-pushed the 4769_fix branch 5 times, most recently from ce9ea78 to 1d28ca9 Compare March 18, 2026 05:10
@JerryWu1234 JerryWu1234 requested review from a team as code owners March 18, 2026 05:10
@Varixo
Copy link
Member

Varixo commented Mar 18, 2026

@Varixo so we should wrap only things that aren't referenced in the component, right?

For example, if a component uses foo.bar in the render function and in the jsx it also uses foo.bar, then we don't need to wrap

but if it uses untrack(() => foo.bar) then we should wrap

Fnsignal is jsx only, no?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@wmertens wmertens wmertens requested changes

Assignees

@JerryWu1234 JerryWu1234

Labels

V2

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@JerryWu1234 @wmertens @Varixo