fix: improve tags api interop support - #425
Conversation
🦋 Changeset detectedLatest commit: e6b57c6 The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 packages
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 |
WalkthroughThis pull request updates dependencies across the monorepo including TypeScript, ESLint, Prettier, and build tools. It modifies the GitHub Actions CI workflow to use token-based permissions instead of environment variables. The changes introduce interop support to the language-tools package by refactoring the script extraction logic and runtime API detection. A test fixture is updated to rename a property from Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/language-server/package.json (1)
9-29: Language‑server dependency bumps; watch@types/jsdomvsjsdomalignmentThe upgrades to
@marko/compiler,marko,prettier,typescript,vscode-css-languageservice,axe-core, andtsxare in line with the rest of the repo and should mainly be tooling/runtime refreshes. The only potential edge is@types/jsdomnow at 27.0.0 whilejsdomremains on ^26.1.0—please confirm those versions are compatible (or consider dropping the separate types ifjsdomships its own) and run the language‑server test suite to catch any regressions.packages/language-tools/src/extractors/script/util/get-runtime-api.ts (1)
37-71: File/AST‑based API heuristics look good; consider a couple of edge cases and small cleanups.
getTagsDir’s right‑to‑left scan correctly prefers a trailing/tagssegment and bails out when it encounters a/componentssegment, but note that a path like.../tags/.../components/...will resolve tofalse(class API) because the deepercomponentswins—if you ever rely on “any ancestortagsdirectory implies tags API,” you may want to revisit that. Also, indetectAPIFromTag, you repeatedly callparsed.read(attr.name)and allocate a fresh/:/RegExp per attribute; hoisting the read into a local and the regex into a module‑level constant would slightly simplify and cheapen this hot path.Also applies to: 124-172
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (82)
package-lock.jsonis excluded by!**/package-lock.jsonand included by**packages/language-server/src/__tests__/fixtures/html/accesskeys/__snapshots__/accesskeys.expected/index.tsis excluded by!**/__snapshots__/**and included by**packages/language-server/src/__tests__/fixtures/html/alt-text/__snapshots__/alt-text.expected/index.tsis excluded by!**/__snapshots__/**and included by**packages/language-server/src/__tests__/fixtures/html/aria-allowed-attr/__snapshots__/aria-allowed-attr.expected/index.tsis excluded by!**/__snapshots__/**and included by**packages/language-server/src/__tests__/fixtures/html/aria-allowed-role/__snapshots__/aria-allowed-role.expected/index.tsis excluded by!**/__snapshots__/**and included by**packages/language-server/src/__tests__/fixtures/html/aria-hidden-body/__snapshots__/aria-hidden-body.expected/index.tsis excluded by!**/__snapshots__/**and included by**packages/language-server/src/__tests__/fixtures/html/aria-hidden-focus/__snapshots__/aria-hidden-focus.expected/index.tsis excluded by!**/__snapshots__/**and included by**packages/language-server/src/__tests__/fixtures/html/aria-roles/__snapshots__/aria-roles.expected/index.tsis excluded by!**/__snapshots__/**and included by**packages/language-server/src/__tests__/fixtures/html/aria-text/__snapshots__/aria-text.expected/index.tsis excluded by!**/__snapshots__/**and included by**packages/language-server/src/__tests__/fixtures/html/autocomplete-valid/__snapshots__/autocomplete-valid.expected/index.tsis excluded by!**/__snapshots__/**and included by**packages/language-server/src/__tests__/fixtures/html/avoid-inline-spacing/__snapshots__/avoid-inline-spacing.expected/index.tsis excluded by!**/__snapshots__/**and included by**packages/language-server/src/__tests__/fixtures/html/blink/__snapshots__/blink.expected/index.tsis excluded by!**/__snapshots__/**and included by**packages/language-server/src/__tests__/fixtures/html/empty-heading/__snapshots__/empty-heading.expected/index.tsis excluded by!**/__snapshots__/**and included by**packages/language-server/src/__tests__/fixtures/html/form-field-multiple-labels/__snapshots__/form-field-multiple-labels.expected/index.tsis excluded by!**/__snapshots__/**and included by**packages/language-server/src/__tests__/fixtures/html/html-lang-valid/__snapshots__/html-lang-valid.expected/index.tsis excluded by!**/__snapshots__/**and included by**packages/language-server/src/__tests__/fixtures/html/html-xml-lang-mismatch/__snapshots__/html-xml-lang-mismatch.expected/index.tsis excluded by!**/__snapshots__/**and included by**packages/language-server/src/__tests__/fixtures/html/landmark-unique/__snapshots__/landmark-unique.expected/index.tsis excluded by!**/__snapshots__/**and included by**packages/language-server/src/__tests__/fixtures/html/marquee/__snapshots__/marquee.expected/index.tsis excluded by!**/__snapshots__/**and included by**packages/language-server/src/__tests__/fixtures/html/presentation-role-conflict/__snapshots__/presentation-role-conflict.expected/index.tsis excluded by!**/__snapshots__/**and included by**packages/language-server/src/__tests__/fixtures/html/tabindex/__snapshots__/tabindex.expected/index.tsis excluded by!**/__snapshots__/**and included by**packages/language-server/src/__tests__/fixtures/html/valid-lang/__snapshots__/valid-lang.expected/index.tsis excluded by!**/__snapshots__/**and included by**packages/language-server/src/__tests__/fixtures/script/abort-signal/__snapshots__/abort-signal.expected/index.tsis excluded by!**/__snapshots__/**and included by**packages/language-server/src/__tests__/fixtures/script/attr-class-id-shorthands/__snapshots__/attr-class-id-shorthands.expected/index.tsis excluded by!**/__snapshots__/**and included by**packages/language-server/src/__tests__/fixtures/script/attr-tag-target-property/__snapshots__/attr-tag-target-property.expected/index.tsis excluded by!**/__snapshots__/**and included by**packages/language-server/src/__tests__/fixtures/script/attr-tags-dynamic-for/__snapshots__/attr-tags-dynamic-for.expected/index.tsis excluded by!**/__snapshots__/**and included by**packages/language-server/src/__tests__/fixtures/script/attr-tags-dynamic-if/__snapshots__/attr-tags-dynamic-if.expected/index.tsis excluded by!**/__snapshots__/**and included by**packages/language-server/src/__tests__/fixtures/script/attr-tags-dynamic-while/__snapshots__/attr-tags-dynamic-while.expected/index.tsis excluded by!**/__snapshots__/**and included by**packages/language-server/src/__tests__/fixtures/script/attr-tags-dynamic-with-scriptlets/__snapshots__/attr-tags-dynamic-with-scriptlets.expected/index.tsis excluded by!**/__snapshots__/**and included by**packages/language-server/src/__tests__/fixtures/script/attr-tags-params-js/__snapshots__/attr-tags-params-js.expected/index.jsis excluded by!**/__snapshots__/**and included by**packages/language-server/src/__tests__/fixtures/script/attr-tags-params/__snapshots__/attr-tags-params.expected/index.tsis excluded by!**/__snapshots__/**and included by**packages/language-server/src/__tests__/fixtures/script/attr-tags-static-dynamic-renderbody/__snapshots__/attr-tags-static-dynamic-renderbody.expected/index.tsis excluded by!**/__snapshots__/**and included by**packages/language-server/src/__tests__/fixtures/script/attr-tags-static/__snapshots__/attr-tags-static.expected/index.tsis excluded by!**/__snapshots__/**and included by**packages/language-server/src/__tests__/fixtures/script/basic/__snapshots__/basic.expected/index.tsis excluded by!**/__snapshots__/**and included by**packages/language-server/src/__tests__/fixtures/script/bound-attr-const-dynamic-member/__snapshots__/bound-attr-const-dynamic-member.expected/index.tsis excluded by!**/__snapshots__/**and included by**packages/language-server/src/__tests__/fixtures/script/bound-attr-const-ident/__snapshots__/bound-attr-const-ident.expected/index.tsis excluded by!**/__snapshots__/**and included by**packages/language-server/src/__tests__/fixtures/script/bound-attr-const-literal-member/__snapshots__/bound-attr-const-literal-member.expected/index.tsis excluded by!**/__snapshots__/**and included by**packages/language-server/src/__tests__/fixtures/script/bound-attr-const-with-type/__snapshots__/bound-attr-const-with-type.expected/index.tsis excluded by!**/__snapshots__/**and included by**packages/language-server/src/__tests__/fixtures/script/bound-attr-missing-ident/__snapshots__/bound-attr-missing-ident.expected/index.tsis excluded by!**/__snapshots__/**and included by**packages/language-server/src/__tests__/fixtures/script/bound-attr-mut-dynamic-member/__snapshots__/bound-attr-mut-dynamic-member.expected/index.tsis excluded by!**/__snapshots__/**and included by**packages/language-server/src/__tests__/fixtures/script/bound-attr-mut-ident/__snapshots__/bound-attr-mut-ident.expected/index.tsis excluded by!**/__snapshots__/**and included by**packages/language-server/src/__tests__/fixtures/script/bound-attr-mut-literal-member/__snapshots__/bound-attr-mut-literal-member.expected/index.tsis excluded by!**/__snapshots__/**and included by**packages/language-server/src/__tests__/fixtures/script/class-api-basic/__snapshots__/class-api-basic.expected/components/fancy-button/index.tsis excluded by!**/__snapshots__/**and included by**packages/language-server/src/__tests__/fixtures/script/class-api-basic/__snapshots__/class-api-basic.expected/index.tsis excluded by!**/__snapshots__/**and included by**packages/language-server/src/__tests__/fixtures/script/class-api-with-component-auto-import/__snapshots__/class-api-with-component-auto-import.expected/index.tsis excluded by!**/__snapshots__/**and included by**packages/language-server/src/__tests__/fixtures/script/class-api-with-pojo-component-auto-import/__snapshots__/class-api-with-pojo-component-auto-import.expected/index.tsis excluded by!**/__snapshots__/**and included by**packages/language-server/src/__tests__/fixtures/script/dynamic-tag/__snapshots__/dynamic-tag.expected/index.tsis excluded by!**/__snapshots__/**and included by**packages/language-server/src/__tests__/fixtures/script/empty-tagname/__snapshots__/empty-tagname.expected/index.tsis excluded by!**/__snapshots__/**and included by**packages/language-server/src/__tests__/fixtures/script/for-tag-attr-tags/__snapshots__/for-tag-attr-tags.expected/index.tsis excluded by!**/__snapshots__/**and included by**packages/language-server/src/__tests__/fixtures/script/for-tag-input/__snapshots__/for-tag-input.expected/index.tsis excluded by!**/__snapshots__/**and included by**packages/language-server/src/__tests__/fixtures/script/for-tag/__snapshots__/for-tag.expected/index.tsis excluded by!**/__snapshots__/**and included by**packages/language-server/src/__tests__/fixtures/script/if-tag/__snapshots__/if-tag.expected/index.tsis excluded by!**/__snapshots__/**and included by**packages/language-server/src/__tests__/fixtures/script/input-type-generic-with-return/__snapshots__/input-type-generic-with-return.expected/index.tsis excluded by!**/__snapshots__/**and included by**packages/language-server/src/__tests__/fixtures/script/invalid-attrs/__snapshots__/invalid-attrs.expected/components/fancy-button/index.tsis excluded by!**/__snapshots__/**and included by**packages/language-server/src/__tests__/fixtures/script/invalid-attrs/__snapshots__/invalid-attrs.expected/index.tsis excluded by!**/__snapshots__/**and included by**packages/language-server/src/__tests__/fixtures/script/native-tag-var-multi-level-hoisting/__snapshots__/native-tag-var-multi-level-hoisting.expected/index.tsis excluded by!**/__snapshots__/**and included by**packages/language-server/src/__tests__/fixtures/script/prefer-local-identifier-tag-name/__snapshots__/prefer-local-identifier-tag-name.expected/index.tsis excluded by!**/__snapshots__/**and included by**packages/language-server/src/__tests__/fixtures/script/recursive-input-scope-hoist/__snapshots__/recursive-input-scope-hoist.expected/index.tsis excluded by!**/__snapshots__/**and included by**packages/language-server/src/__tests__/fixtures/script/render-body-basic/__snapshots__/render-body-basic.expected/index.tsis excluded by!**/__snapshots__/**and included by**packages/language-server/src/__tests__/fixtures/script/return-tag-basic/__snapshots__/return-tag-basic.expected/index.tsis excluded by!**/__snapshots__/**and included by**packages/language-server/src/__tests__/fixtures/script/return-tag-nested/__snapshots__/return-tag-nested.expected/index.mdis excluded by!**/__snapshots__/**and included by**packages/language-server/src/__tests__/fixtures/script/return-tag-nested/__snapshots__/return-tag-nested.expected/index.tsis excluded by!**/__snapshots__/**and included by**packages/language-server/src/__tests__/fixtures/script/script/__snapshots__/script.expected/index.tsis excluded by!**/__snapshots__/**and included by**packages/language-server/src/__tests__/fixtures/script/simple-hoist/__snapshots__/simple-hoist.expected/index.tsis excluded by!**/__snapshots__/**and included by**packages/language-server/src/__tests__/fixtures/script/split-component-d-ts/__snapshots__/split-component-d-ts.expected/components/fancy-button/index.d.tsis excluded by!**/__snapshots__/**and included by**packages/language-server/src/__tests__/fixtures/script/split-component-d-ts/__snapshots__/split-component-d-ts.expected/components/fancy-button/index.tsis excluded by!**/__snapshots__/**and included by**packages/language-server/src/__tests__/fixtures/script/split-component-d-ts/__snapshots__/split-component-d-ts.expected/components/regular-button/index.d.tsis excluded by!**/__snapshots__/**and included by**packages/language-server/src/__tests__/fixtures/script/split-component-d-ts/__snapshots__/split-component-d-ts.expected/components/regular-button/index.tsis excluded by!**/__snapshots__/**and included by**packages/language-server/src/__tests__/fixtures/script/split-component-d-ts/__snapshots__/split-component-d-ts.expected/index.tsis excluded by!**/__snapshots__/**and included by**packages/language-server/src/__tests__/fixtures/script/tag-param-mutation/__snapshots__/tag-param-mutation.expected/index.tsis excluded by!**/__snapshots__/**and included by**packages/language-server/src/__tests__/fixtures/script/tag-params-basic/__snapshots__/tag-params-basic.expected/index.tsis excluded by!**/__snapshots__/**and included by**packages/language-server/src/__tests__/fixtures/script/tag-params-syntax-error/__snapshots__/tag-params-syntax-error.expected/index.tsis excluded by!**/__snapshots__/**and included by**packages/language-server/src/__tests__/fixtures/script/tag-params-with-generics/__snapshots__/tag-params-with-generics.expected/index.tsis excluded by!**/__snapshots__/**and included by**packages/language-server/src/__tests__/fixtures/script/tag-type-params/__snapshots__/tag-type-params.expected/components/test-tag.tsis excluded by!**/__snapshots__/**and included by**packages/language-server/src/__tests__/fixtures/script/tag-type-params/__snapshots__/tag-type-params.expected/index.tsis 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.tsis excluded by!**/__snapshots__/**and included by**packages/language-server/src/__tests__/fixtures/script/tag-var-hoisting/__snapshots__/tag-var-hoisting.expected/index.tsis excluded by!**/__snapshots__/**and included by**packages/language-server/src/__tests__/fixtures/script/tag-var-mutation-shadowing/__snapshots__/tag-var-mutation-shadowing.expected/index.tsis excluded by!**/__snapshots__/**and included by**packages/language-server/src/__tests__/fixtures/script/tag-var-mutations-nested/__snapshots__/tag-var-mutations-nested.expected/index.tsis excluded by!**/__snapshots__/**and included by**packages/language-server/src/__tests__/fixtures/script/tag-var-mutations/__snapshots__/tag-var-mutations.expected/index.tsis excluded by!**/__snapshots__/**and included by**packages/language-server/src/__tests__/fixtures/script/tag-var-syntax-error/__snapshots__/tag-var-syntax-error.expected/index.tsis excluded by!**/__snapshots__/**and included by**packages/language-server/src/__tests__/fixtures/script/tags-api-basic/__snapshots__/tags-api-basic.expected/components/fancy-button/index.tsis excluded by!**/__snapshots__/**and included by**packages/language-server/src/__tests__/fixtures/script/tags-api-basic/__snapshots__/tags-api-basic.expected/index.tsis excluded by!**/__snapshots__/**and included by**
📒 Files selected for processing (12)
.changeset/easy-lions-cut.md(1 hunks).github/workflows/ci.yml(1 hunks)package.json(1 hunks)packages/language-server/package.json(1 hunks)packages/language-server/src/__tests__/fixtures/script/tag-type-params/components/test-tag.marko(1 hunks)packages/language-server/src/__tests__/index.test.ts(1 hunks)packages/language-tools/marko.internal.d.ts(1 hunks)packages/language-tools/package.json(1 hunks)packages/language-tools/src/extractors/script/index.ts(10 hunks)packages/language-tools/src/extractors/script/util/get-runtime-api.ts(5 hunks)packages/type-check/package.json(1 hunks)packages/vscode/package.json(1 hunks)
🔇 Additional comments (13)
packages/vscode/package.json (1)
105-113: Dev tooling version bumps for VSCode client look fineOnly devDependencies are updated and remain aligned with the rest of the repo; no runtime surface is affected. Please just ensure your extension build/release flow still works with the newer
vsce,ovsx, andtsxCLIs..github/workflows/ci.yml (1)
44-49: Release job permissions updated; double‑check npm publish authenticationAdding
id-token: writeandcontents: readmatches GitHub’s OIDC recommendations forchangesets/action. However, withNPM_TOKENno longer present in this job, please confirm thatnpm run release(and any underlyingnpm publish) still has valid credentials configured (for example via another env var or OIDC‑based auth) so automated releases don’t start failing.packages/language-server/src/__tests__/index.test.ts (1)
13-17: Non‑functional change tointernalTypesFileinitializationThis is just a formatting/structural tweak around
require.resolveand keeps the same resolved path, so test behavior is unchanged.packages/type-check/package.json (1)
6-13: TypeScript dependency bumped to 5.9.3Patch‑level TS bump keeps this CLI aligned with the rest of the monorepo’s toolchain; no issues expected, but please rerun the type‑check CLI against a representative project to catch any subtle diagnostic changes.
packages/language-tools/package.json (1)
9-23: Language‑tools parser/compiler/tooling bumps look consistent
@babel/parser,@marko/compiler,@typescript/vfs,marko, andtsxare all aligned with the versions used elsewhere in the repo, so this should mainly be a tooling refresh. It’s worth running the language‑tools test suite and a quick manual pass over script extraction to ensure no regressions from the new parser/compiler versions.packages/language-server/src/__tests__/fixtures/script/tag-type-params/components/test-tag.marko (1)
1-4: Fixture now modelscontentinstead ofrenderBodySwitching the
Inputinterface to use acontent: Marko.Body<[T], U>property matches the tags‑API style body key and lines up with the newcontentFortyping. This keeps the tag‑type‑params fixture representative of current runtime APIs.packages/language-tools/marko.internal.d.ts (1)
39-49:Marko._.contentForadds anany/unknown guard before API‑based inferenceThe new
[0] extends [1 & Name] ? DefaultBodyContentKey : …branch mirrors the pattern used elsewhere in this file to treatany/unknown inputs as falling back toDefaultBodyContentKey, while preserving the existing"tags" → "content"and"class" → "renderBody"behavior for well‑typed tags. This should makecontentForsafer under loose generics without changing behavior for correctly typed callers..changeset/easy-lions-cut.md (1)
1-8: Changeset contents match the PR scope.Patch bumps for the four packages and the “Improve tags api interop support.” summary line are clear and aligned with the code changes.
package.json (1)
4-21: Tooling version bumps look coherent; confirm Node/CI compatibility.The eslint/@eslint-js 9.x, TypeScript 5.9.x, lint-staged 16.x, mocha 11.x, etc., upgrades are consistent, but they expect a fairly recent Node runtime (typically ≥18). Please double‑check your CI and any documented engine constraints line up with these versions.
packages/language-tools/src/extractors/script/util/get-runtime-api.ts (1)
5-27: Runtime API detection + interop flag wiring looks consistent; double‑check translator semantics.Deriving
interopfrom the absence of a translator‑levelpreferAPI === "tags"and then letting filename/program/body heuristics choose the concreteapigives you “interop by default, opt‑out via tags preference,” which matches the PR’s intent. Just be aware that if translators ever start emitting"class"or other explicit values, they’ll still be treated as interop (sincedetectAPIFromTranslatoronly handles"tags"), not as a “non‑interop class” mode—worth confirming that this is exactly the behavior you want going forward.Also applies to: 29-36, 73-109, 111-122
packages/language-tools/src/extractors/script/index.ts (3)
109-148: Early runtime API/interop derivation in the constructor looks solid.Computing
{ api, interop } = getRuntimeAPI(...)once up front and then usingthis.#api === RuntimeAPI.classto gate component filename resolution and Component generation cleanly separates class‑API vs tags‑API code paths and avoids leaking class scaffolding into tags‑only templates.Also applies to: 157-161, 226-276, 300-321
330-355: Class‑only locals and conditionalapioverride keep interop vs non‑interop nicely separated.Restricting
component/state/outinitialization and their inclusion in thenoopcall tothis.#api === RuntimeAPI.classprevents undefined‑variable usage when emitting tags‑API templates. Likewise, only emittingapi: "${this.#api}",intemplateOverrideClasswhenthis.#interopis true lines up with the new runtime‑API contract so pure tags projects don’t get anapioverride at all.Also applies to: 369-371, 379-415, 394-405
1003-1006: Tag input/body interop (args, events, andcontentFor) looks coherent; confirm a few assumptions.Gating event‑handler binding on
this.#api === RuntimeAPI.classin#writeAttrsis important now thatcomponentonly exists for class API, and it avoids class‑style binding from being applied in tags‑API code. In#writeTagInputObject, forwardingtag.argspositionally only whenthis.#api === RuntimeAPI.tagsor the tag name is fully dynamic, and otherwise wrapping args into the"value"slot viatuple(...), matches the expected tags vs class calling conventions but is a subtle change—worth validating against any existing fixtures that rely on argument shape. For body content, the newthis.#interopbranch that prefers[Marko._.contentFor(tag)]when atagIdis available and falls back to[Marko._.content]/"content"otherwise seems right; just ensure#ensureTagIdis hit for any tags whose attr tags you expect to be interop‑aware socontentForis actually used.Also applies to: 1251-1264, 1291-1302, 1311-1327, 1369-1382
No description provided.