Skip to content

fix: hoisted tag vars no longer collapse to never - #582

Open
briancarbone wants to merge 1 commit into
marko-js:mainfrom
briancarbone:main
Open

fix: hoisted tag vars no longer collapse to never#582
briancarbone wants to merge 1 commit into
marko-js:mainfrom
briancarbone:main

Conversation

@briancarbone

Copy link
Copy Markdown

@changeset-bot

changeset-bot Bot commented Jul 30, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: fac45fb

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 5 packages
Name Type
@marko/language-server Patch
@marko/language-tools Patch
@marko/ts-plugin Patch
@marko/type-check Patch
marko-vscode Patch

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

@coderabbitai

coderabbitai Bot commented Jul 30, 2026

Copy link
Copy Markdown

Review Change Stack

Walkthrough

The change updates Marko._.hoist so non-function inputs resolve to Exclude<T, undefined> instead of never. The style tag-variable fixture adds a top-level styles.button usage with a diagnostic marker. A changeset records patch releases for related packages, while cleanup and performance notes document program-binding source information and unconditional tag-variable hoisting.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly describes the main fix: hoisted tag vars no longer infer as never.
Description check ✅ Passed The description matches the change and linked issue, noting the fix and test updates.
Linked Issues check ✅ Passed The type fix and updated fixture address the never inference bug while keeping class access checks.
Out of Scope Changes check ✅ Passed The remaining changes are supporting tests/docs and a changeset for the same fix, with no unrelated code introduced.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Nitpick comments (1)
packages/language-server/src/__tests__/fixtures/script/style-tag-var/index.marko (1)

1-2: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Add HTML-syntax coverage for the hoisted style variable.

This fixture only exercises concise syntax, while the PR objective requires both concise and HTML syntax. Add an equivalent non-self-closing HTML form with a styles.button marker.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@packages/language-server/src/__tests__/fixtures/script/style-tag-var/index.marko`
around lines 1 - 2, Extend the style variable fixture alongside the existing
styles.button concise-syntax case with an equivalent non-self-closing HTML-form
element and styles.button marker. Preserve the current concise fixture and
ensure both syntax forms are covered.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@agent-feedback/cleanup.md`:
- Around line 4-9: Record both documented follow-ups in
agent-feedback/README.md: add the ProgramBinding source-node/tag-kind plumbing
item from agent-feedback/cleanup.md lines 4-9, and add the tag-variable hoisting
performance item from agent-feedback/perf.md lines 4-9. Preserve the existing
feedback details and link each README entry to its corresponding document.

---

Nitpick comments:
In
`@packages/language-server/src/__tests__/fixtures/script/style-tag-var/index.marko`:
- Around line 1-2: Extend the style variable fixture alongside the existing
styles.button concise-syntax case with an equivalent non-self-closing HTML-form
element and styles.button marker. Preserve the current concise fixture and
ensure both syntax forms are covered.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 00f3e69a-14e3-4c5d-a13e-24d708580f29

📥 Commits

Reviewing files that changed from the base of the PR and between 832e53c and fac45fb.

⛔ Files ignored due to path filters (4)
  • packages/language-server/src/__tests__/fixtures/script/style-tag-var/__snapshots__/style-tag-var.expected/index.html is excluded by !**/__snapshots__/** and included by **
  • packages/language-server/src/__tests__/fixtures/script/style-tag-var/__snapshots__/style-tag-var.expected/index.md is excluded by !**/__snapshots__/** and included by **
  • packages/language-server/src/__tests__/fixtures/script/style-tag-var/__snapshots__/style-tag-var.expected/index.ts is excluded by !**/__snapshots__/** and included by **
  • packages/language-server/src/__tests__/fixtures/script/tag-var-hoisting-from-control-flow/__snapshots__/tag-var-hoisting-from-control-flow.expected/index.md is excluded by !**/__snapshots__/** and included by **
📒 Files selected for processing (5)
  • .changeset/hoisted-tag-var-never.md
  • agent-feedback/cleanup.md
  • agent-feedback/perf.md
  • packages/language-server/src/__tests__/fixtures/script/style-tag-var/index.marko
  • packages/language-tools/marko.internal.d.ts

Comment thread agent-feedback/cleanup.md
Comment on lines +4 to +9

## Plumb the source node (or tag kind) through `getProgramBindings`

`packages/language-tools/src/extractors/script/util/attach-scopes.ts` › `getProgramBindings` | 2026-07-30 | impact:low | effort:med

`ProgramBinding` exposes only `{ name, sources }` (plus mutation info), dropping the declaring node/tag. This makes it impossible for the script extractor's program-level hoist emission (`packages/language-tools/src/extractors/script/index.ts` › `#writeProgram`) to treat any tag kind specially — e.g. declaring a `<style/styles>` CSS-module var with its statically known selector type at program scope instead of routing it through `Marko._.hoist`. If per-tag-kind program-level typing is ever wanted, the binding needs to carry its `Node.Tag` (or at least the tag name). Re-verify: inspect the `ProgramBinding` interface in attach-scopes.ts and confirm it has no node reference.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Record actionable feedback in agent-feedback/README.md. Both additions document out-of-scope follow-ups but omit the required README record.

  • agent-feedback/cleanup.md#L4-L9: add the ProgramBinding follow-up to agent-feedback/README.md.
  • agent-feedback/perf.md#L4-L9: add the tag-variable hoisting performance follow-up to agent-feedback/README.md.
📍 Affects 2 files
  • agent-feedback/cleanup.md#L4-L9 (this comment)
  • agent-feedback/perf.md#L4-L9
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@agent-feedback/cleanup.md` around lines 4 - 9, Record both documented
follow-ups in agent-feedback/README.md: add the ProgramBinding
source-node/tag-kind plumbing item from agent-feedback/cleanup.md lines 4-9, and
add the tag-variable hoisting performance item from agent-feedback/perf.md lines
4-9. Preserve the existing feedback details and link each README entry to its
corresponding document.

Source: Coding guidelines

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Scoped <style/name> tag variable is typed never

1 participant