-
Notifications
You must be signed in to change notification settings - Fork 665
Fix hoisting and unify custom tag hoists with native tag refs #3064
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
🦋 Changeset detectedLatest commit: b736b2b The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3064 +/- ##
==========================================
- Coverage 89.04% 89.03% -0.02%
==========================================
Files 375 375
Lines 47452 47496 +44
Branches 4102 4128 +26
==========================================
+ Hits 42254 42286 +32
- Misses 5145 5158 +13
+ Partials 53 52 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
WalkthroughRefactors hoisting internals in packages/runtime-tags: replaces 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
`@packages/runtime-tags/src/__tests__/fixtures/controllable-checked-many/template.marko`:
- Line 1: Remove the unused Node-only import "import { console } from
\"node:inspector\";" from the template fixture so it no longer pulls in
Node-only APIs that break browser/DOM builds; locate the import statement
referencing "console" and "node:inspector" at the top of the file and delete
that line, leaving the rest of the template untouched.
In
`@packages/runtime-tags/src/__tests__/fixtures/controllable-checked-many/test.ts`:
- Around line 3-9: The helper functions click0 and click1 call .click() on
container.querySelectorAll("input").item(0/item(1)) without checking for null;
add an explicit guard in each function to check the result of .item(...) and
either throw a clear error (e.g., "expected input `#0` not found") or return
safely before calling .click(), so the test failure is explicit if the fixture
markup changes. Ensure you update both functions (click0 and click1).
🧹 Nitpick comments (1)
packages/runtime-tags/src/__tests__/fixtures/hoist-return-ref/test.ts (1)
1-5: Consider removing commented-out code.The commented-out
clickfunction and additional steps could be removed if not needed for this test. If they're intended for future use, consider adding a TODO comment explaining the plan.🧹 Proposed cleanup
-export const steps = [{}]; //, click, click, click]; - -// function click(container: Element) { -// container.querySelector("button")!.click(); -// } +export const steps = [{}];
packages/runtime-tags/src/__tests__/fixtures/controllable-checked-many/template.marko
Outdated
Show resolved
Hide resolved
packages/runtime-tags/src/__tests__/fixtures/controllable-checked-many/test.ts
Show resolved
Hide resolved
32d170b to
2a8f688
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@packages/runtime-tags/src/__tests__/fixtures/spread-attr-tag-effect/tags/child.marko`:
- Around line 1-2: Guard against input.option being null/undefined in the script
that sets $el().innerHTML: when computing Object.keys(input.option) (inside the
script block assigning $el().innerHTML), default input.option to an empty object
(e.g., use input.option || {} or nullish coalescing) before calling Object.keys
so the code won't throw if option is missing.
🧹 Nitpick comments (1)
packages/runtime-tags/src/__tests__/fixtures/hoist-native-tag-var/template.marko (1)
23-28: Consider using a distinct variable name to improve readability.Both script blocks declare
const el(line 11 and line 24). While Marko scopes each<script>block separately so this is technically correct, using distinct names likeel1andel2(matching the$el/$el2references) would improve clarity and make the test fixture easier to follow.✨ Suggested improvement
<script> - const el = $el2() - if (el) { - el.innerHTML = 'Hello World'; + const el2 = $el2() + if (el2) { + el2.innerHTML = 'Hello World'; } </script>
packages/runtime-tags/src/__tests__/fixtures/spread-attr-tag-effect/tags/child.marko
Show resolved
Hide resolved
2a8f688 to
1c565a9
Compare
1c565a9 to
b736b2b
Compare
No description provided.