-
Notifications
You must be signed in to change notification settings - Fork 12
agent-feedback: attribute tag params, imported templates and namespaced attrs #595
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,12 @@ | ||
| --- | ||
| type: bug | ||
| impact: med | ||
| effort: med | ||
| site: packages/type-check/src/run.ts › compilerHost.resolveModuleNameLiterals | ||
| --- | ||
|
|
||
| # Honour a package's `exports` map when resolving a `.marko` subpath in mtc and the TS plugin | ||
|
|
||
| `resolveModuleNameLiterals` decides from the specifier's own text, taking its `.marko` branch only when the specifier carries a processor extension, so a bare specifier such as `@acme/ui/tags/acme-modal` falls through to `ts.bundlerModuleNameResolver`, which does honour `exports` but accepts only TS/JS/JSON targets. An `exports` pattern like `"./tags/*": "./dist/tags/*/index.marko"` is therefore TS2307 under `mtc` while Vite resolves it and builds, and a library author has no package.json-side workaround, since a `"types": "./dist/tags/*/index.d.marko"` condition is TS2307 as well. The cost is worse than the one diagnostic: the import becomes `any`, so the tag's attributes stop being checked (a bogus attribute is accepted silently) and `noImplicitAny` fires on its event handler parameters instead. The resolver is wrong in the other direction too, because the `.marko` node-module branch resolves `<pkg>/package.json` and then joins the remaining subpath literally onto that directory, so a deep path that the package's `exports` does not expose type-checks clean and then fails the build with `Package subpath './dist/tags/acme-modal/index.marko' is not defined by "exports"`. Run the specifier through Node/TS `exports` resolution first and let the Marko processor claim the resulting file by its extension, rather than deciding on the specifier's extension; the same resolver is duplicated in `packages/language-server/src/ts-plugin/host.ts`, so a fix has to land in both (its comment there also says it resolves the module's `marko.json` while the code resolves `package.json`). | ||
|
|
||
| Check: in a project on `@marko/type-check` 3.2.0 depending on a package whose `exports` has `"./tags/*": "./dist/tags/*/index.marko"`, write `import ModalA from "@acme/ui/tags/acme-modal"` next to `import ModalB from "@acme/ui/dist/tags/acme-modal/index.marko"` and `static const b: number = ModalB`. `npx mtc -d condensed` reports `error TS2307 Cannot find module '@acme/ui/tags/acme-modal' or its corresponding type declarations` plus `TS2322 Type 'Template' is not assignable to type 'number'` on the `ModalB` line only, and rendering `<ModalA totallyBogus="x" onClose(reason){}>` reports only `TS7006 Parameter 'reason' implicitly has an 'any' type` where `<ModalB ...>` reports TS2353 on `"totallyBogus"`; `npx vite build --app` on the same file builds clean. Both import forms should type-check identically. For the reverse case, drop `"./dist/*"` from the package's `exports`: the `ModalB` import still type-checks under `mtc` and the Vite build then fails with `[plugin marko-vite:pre] Error: Package subpath './dist/tags/acme-modal/index.marko' is not defined by "exports"`. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,12 @@ | ||
| --- | ||
| type: bug | ||
| impact: high | ||
| effort: med | ||
| site: packages/language-tools/marko.internal.d.ts › DynamicRenderer | ||
| --- | ||
|
|
||
| # Restore `TemplateRenderer` for a statically known template in `DynamicRenderer` without losing 5e05dffd's perf win | ||
|
|
||
| `#writeTag` flips `isTemplate` only on the branch where a kebab tag name resolves to a `.marko` file, so every identifier tag name is emitted as `Marko._.renderDynamicTag` and picks up `DynamicRenderer` rather than `TemplateRenderer`; that covers `<Report>` after `import Report from "./report.marko"`, the documented `import Report from "<report>"` shorthand and the explicit `<${Report}/>` form alike. Commit 5e05dffd ("fix: improve performance for dynamic input intersections") merged `DynamicRenderer`'s template and string branches into one `<Input extends Marko.Input<Name>>(input: Marko.Directives & Input)` signature, dropping the reuse of the template's own `_()` renderer and, with it, three checks the discovered spelling of the same tag still gets. Excess-property checking disappears as soon as the call site supplies attributes that satisfy `Marko.Input<Name>`, because `Input` then infers as the call-site literal and freshness has nothing left to reject, so `<Flat title="x" bogus=1/>` and `<Flat title="x"><@nope title="a"/></Flat>` are silent where `<flat-tag title="x" bogus=1/>` reports TS2353; generic parameters are lost, because `Marko.Input<Marko.Template<Input<T>>>` cannot recover `T`, so `<Gen|{ item }| items=[{ id: "c" }]>` is TS18046 where the discovered tag infers `{ id: string }`; and expanding `Marko.Input<>` over a self-referential attr-tag type (`group?: Marko.AttrTag<GroupAttrs>` inside `GroupAttrs`) trips TS2589 on top of the real diagnostic. Missing required attributes and mistyped attribute values are still caught, and a call site whose attributes all fail the constraint still errors, which is why the `tags-api-basic` fixture kept passing and the hole survived the commit. Prefer `Name extends { _: infer Renderer } ? Renderer` for the statically known template case, the way `TemplateRenderer` already does, and keep 5e05dffd's merged intersection shape for genuinely dynamic names. | ||
|
|
||
| Check: in a project on `@marko/type-check` 3.2.0 with a discoverable `tags/` dir holding `flat-tag.marko` (`export interface Input { title: string; count?: number }`), `gen-tag.marko` (`export interface Input<T> { items: T[]; content: Marko.Body<[{ item: T; index: number }]> }`) and `rec-tag.marko` (`export interface GroupAttrs { title: string; group?: Marko.AttrTag<GroupAttrs> }` plus `export interface Input { title: string; group?: Marko.AttrTag<GroupAttrs> }`), render each tag twice from one template, once by discovered name and once through `import Flat from "./tags/flat-tag.marko"` and friends: `<flat-tag title="x" bogus=1/>` vs `<Flat title="x" bogus=1/>`, `<flat-tag title="x"><@nope title="a"/></flat-tag>` vs the `<Flat>` form, `<gen-tag|{ item }| items=[{ id: "a" }]>${item.id}</gen-tag>` vs the `<Gen>` form, and `<rec-tag/>` vs `<Rec/>`. `npx mtc -p tsconfig.json -d condensed` today reports TS2353 on the discovered forms only, TS18046 `'item' is of type 'unknown'` on the imported generic only, and TS2589 `Type instantiation is excessively deep and possibly infinite` on the imported recursive one only; each pair should report the same thing, and patching just the `Marko.Template` case back to `TemplateRenderer<Name>` makes all four agree. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,12 @@ | ||
| --- | ||
| type: bug | ||
| impact: med | ||
| effort: low | ||
| site: packages/language-tools/src/extractors/script/index.ts › ScriptExtractor#getNamedAttrModifier | ||
| --- | ||
|
|
||
| # Split a named attribute at `:` only for bound attrs and the class API, so namespaced names survive into the extracted TS | ||
|
|
||
| `#getNamedAttrModifier` splits every named attribute at its last `:` and `#writeAttrs` then writes only the part before the colon as the object key, although a modifier means something in just two places: the class-API `id:scoped` / `class:no-update`, and bound attributes such as `value:parseInt:=b`. marko-js/marko does the opposite for a non-bound attribute, re-joining `attr.name += ":" + attr.modifier` in `packages/runtime-tags/src/translator/visitors/program/pre-analyze.ts`, and the compiled HTML keeps `xmlns:v=...` and `xml:space=preserve` verbatim, so this split exists only in the extraction and every namespaced attribute collapses to its prefix there. `xml:space` becomes the key `"xml"`, which makes the `"xml:space"` key marko declares in `packages/runtime-tags/tags-html.d.ts` unreachable and turns a plain `<svg xml:space="preserve"/>` into a TS2353, while `xmlns:v` and `xmlns:o` on an HTML-email root both become `"xmlns"` and report TS1117 ("An object literal cannot have multiple properties with the same name") twice, a message that names the wrong problem and points at the wrong fix. Because the real attribute name never reaches TypeScript, declaration merging on `Marko.HTML.HTML` cannot silence any of it either, so a template carrying namespaced attributes has no user-side workaround short of `// @ts-nocheck`. Gate the split on bound attributes and `#api === RuntimeAPI.class`, matching the translator: `xml:space` then checks clean with no user change, and the xmlns pair becomes one honest TS2353 naming `"xmlns:v"` that a small `Marko.HTML.HTML` augmentation can answer (marko declares no `xmlns:*` key, so an email root still needs that augmentation or an upstream declaration). | ||
|
|
||
| Check: in a project with `@marko/type-check` 3.2.0 and `@marko/language-tools` 2.7.0, write `src/probe/ns.marko` with `<html xmlns="http://www.w3.org/1999/xhtml" xmlns:v="urn:schemas-microsoft-com:vml" xmlns:o="urn:schemas-microsoft-com:office:office"><body/></html>` on line 1 and `<svg xml:space="preserve"/>` on line 2, then run `npx mtc -d condensed`. Today it reports `ns.marko:1:44 - error TS1117`, `ns.marko:1:84 - error TS1117` and `ns.marko:2:6 - error TS2353 Object literal may only specify known properties, and '"xml"' does not exist in type 'Directives & SVG'`; line 2 should report nothing and line 1 at most one TS2353 naming `"xmlns:v"`. Adding a `.d.ts` with `declare global { namespace Marko { namespace HTML { interface HTML { "xmlns:v"?: string; "xmlns:o"?: string } } } }` leaves that output byte-identical, because the key TypeScript sees is `"xmlns"`. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,12 @@ | ||
| --- | ||
| type: bug | ||
| impact: high | ||
| effort: med | ||
| site: packages/language-tools/marko.internal.d.ts › attrTagFor | ||
| --- | ||
|
|
||
| # Restore body-parameter types on an attribute tag repeated under one parent | ||
|
|
||
| When the same attribute tag appears more than once under one parent, every body parameter on it loses its contextual type and reports TS7031/TS7006, while a single `<@item|{ n }|>` under the same parent types `n` correctly. `#writeStaticAttrTags` writes a single attribute tag inline into the parent's input object but routes a repeated one through `Marko._.attrTagFor(...)`, whose `AttrTags` constraint recovers the parent input with `Tag extends InputFor<infer Input>`; `InputFor` is itself a conditional type, so TypeScript cannot infer `Input` through it, the check takes its false branch, the constraint degrades to `Record<Name, Marko.AttrTag<unknown>>`, and no contextual signature reaches the body parameter. This is a regression rather than a limitation: the constraint read `Marko.Input<Tag> extends infer Input` (eager, no inference) and typed these bodies correctly until a857d7fe "fix: attr tag types when any", and restoring the eager form makes the non-generic case clean again. A generic parent loses its body-parameter types the same way, whether `T` is inferred from an attribute or given explicitly as `<a-grid<Product>>`, and there the eager form only improves TS7031 to TS18046, so recovering `T` through `attrTagFor` takes more than reverting that one line. The only workaround is annotating every body parameter at every call site, which defeats the point of the generic; a fix also has to re-record `packages/language-server/src/__tests__/fixtures/script/attr-tags-params`, whose snapshot pins the current `(parameter) data: any` hovers and the three implicit-any diagnostics. | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win Qualify the “only workaround” claim.
🤖 Prompt for AI Agents |
||
|
|
||
| Check: in a project with `@marko/type-check` 3.2.0, with `tags/a-multi.marko` declaring `export interface Input { item?: Marko.AttrTag<{ title: string; content?: Marko.Body<[{ n: number }]> }> }`, type-check a template holding one `<a-multi>` with a single `<@item|{ n }| title="x">${n.toFixed(1)}</@item>` and a second `<a-multi>` with two of them: `npx mtc -p tsconfig.json -d condensed` reports `TS7031 Binding element 'n' implicitly has an 'any' type` on both lines of the repeated pair and nothing on the single one, and should report nothing at all. The generic form is the same shape: with `export interface Input<T> { rows: T[]; column?: Marko.AttrTag<{ header: string; content?: Marko.Body<[{ row: T }]> }> }`, two `<@column|{ row }|>` under one parent report TS7031 on `row` while one does not. | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,12 @@ | ||
| --- | ||
| type: dx | ||
| impact: low | ||
| effort: low | ||
| site: packages/language-tools/src/extractors/script/index.ts › ScriptExtractor#writeTagInputObject | ||
| --- | ||
|
|
||
| # Give `// @ts-ignore` above a tag a line it can suppress in the extracted TS | ||
|
|
||
| A `//` comment above a tag can never suppress a TypeScript diagnostic on that tag's attributes, on any of the three paths the extractor has for such a comment. A file-leading `// @ts-` comment is hoisted by `#writeCommentPragmas` to line 1 of the extracted TS, where the next line is `export interface Input {}`; a comment above a tag without params is copied by `#writeTagInputObject` (`if (!tag.params) this.#writeComments(tag)`) onto the `Marko._.renderNativeTag("input")()()(` call line, so the only thing it covers is the `{` that opens the input object while each attribute is written on a later line; and a comment above a tag with params is written by the `tag.params` branch after the whole attribute object, onto the params arrow. Moving the comment inside the tag directly above the offending attribute does not help, because that comment is dropped from the extraction entirely. So `// @ts-expect-error` above a tag reports `TS2578 Unused '@ts-expect-error' directive` at 1:1 and still reports the attribute error, and the only suppression that works is `// @ts-nocheck` on line 1, which turns off checking for the whole file; a template that hits an extractor-side type bug therefore has no narrow escape hatch. Write a tag's leading comments onto the line before its first attribute so a directive covers them, and name the current limitation in `website/docs/reference/typescript.md`, which mentions `// @ts-nocheck` and no other suppression. | ||
|
|
||
| Check: in a project with `@marko/type-check` 3.2.0, write `src/probe/ign1.marko` as the four lines `// @ts-ignore`, `<input`, ` bogus=1`, `/>`; `src/probe/ign4.marko` the same with `// @ts-expect-error`; `src/probe/ign6.marko` as `<input`, ` // @ts-ignore`, ` bogus=1`, `/>`; and `src/probe/ign9.marko` as `<div>` / ` // @ts-ignore` / ` <for|item| of=[1] bogus=1>` / ` ${item}` / ` </for>` / `</div>`. `npx mtc -d condensed` reports `TS2353 Object literal may only specify known properties, and '"bogus"' does not exist in type 'Directives & Input'` for ign1, ign6 and ign9, and for ign4 both `ign4.marko:1:1 - error TS2578` and the same TS2353; after the fix the directive suppresses the attribute diagnostic and no TS2578 is emitted. |
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.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the inline code span spacing.
Line 16 places two leading spaces inside the inline code span for
<const/rowId=String(row.id)>. Markdownlint reports MD038. Remove the padding or use a fenced code block if the indentation is significant.🧰 Tools
🪛 markdownlint-cli2 (0.23.2)
[warning] 16-16: Spaces inside code span elements
(MD038, no-space-in-code)
🤖 Prompt for AI Agents
Source: Linters/SAST tools