fix: add React-compatible webcomponent setters and type declarations#932
fix: add React-compatible webcomponent setters and type declarations#932pzanella wants to merge 8 commits intomoq-dev:mainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a Custom Elements Manifest–based code generation pipeline and generator tooling (scripts/element-wrappers) including a React wrapper generator and manifest utilities. Converts publish and watch UI modules from Solid.js custom-element wrappers to native Web Components with Shadow DOM and explicit connected/disconnected lifecycle handling. Updates build and package configuration: new prebuild script, added exports for a React wrapper entry, inclusion of custom-elements.json, vite-plugin-dts, and copying the manifest into dist. Adds cem.config.js files and ignores generated wrapper directories and the manifest in .gitignore. 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@js/hang/src/publish/element.ts`:
- Around line 248-265: The JSX namespace augmentations reference
React.HTMLAttributes but React is not imported; add a type-only import of React
at the top of the file (and replicate in js/hang/src/watch/element.ts) so the
JSX IntrinsicElements block can resolve React.HTMLAttributes for "hang-publish"
(the augmentation using HangPublish and the Observed mapped props).
Specifically, insert a type-only import such as importing the React namespace
(e.g., import type * as React from "react";) before the JSX declaration so
TypeScript can find React.HTMLAttributes.
In `@js/hang/src/watch/element.ts`:
- Around line 189-191: The jitter setter currently uses a truthy check which
treats 0 as falsy and removes the attribute; update the set jitter(value: string
| Time.Milli) setter to check for null/undefined instead (e.g., value != null ?
this.setAttribute("jitter", String(value)) : this.removeAttribute("jitter")) so
a numeric 0 is preserved; ensure you change only the setter for "jitter" (and
optionally mirror the same != null pattern for the url/path setters for
consistency) while leaving attributeChangedCallback logic (the 100ms fallback)
intact.
- Around line 212-218: The global JSX namespace currently references
React.HTMLAttributes which forces React types on all consumers; change the
global JSX.IntrinsicElements entry for "hang-watch" to use a framework-agnostic
props shape (e.g., basic HTML attribute type or a plain object plus the
observed-attrs mapping) instead of React.HTMLAttributes, keep the
observed-attribute mapping using the existing Observed union and the
string|number|boolean types, and move the React-specific declaration that uses
React.HTMLAttributes into the existing declare module "react" block (and mirror
the same change in publish/element.ts); update the HangWatch declaration
references so consumers get a neutral global JSX type while React users still
receive the React.HTMLAttributes-augmented signature via the declare module
"react".
🧹 Nitpick comments (1)
js/hang/src/watch/element.ts (1)
173-183: Property getter/setter type asymmetry is intentional but worth documenting.The getters return
Signal<T>while the setters accept primitive values — this is the intended pattern for React interop (React sets properties directly). However, this meanselement.url = element.urlwould be a type error since the getter returns aSignaland the setter expects astring | URL | undefined. A brief JSDoc comment on the setters explaining the React interop purpose would help future maintainers.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@js/hang-ui/src/watch/watch.d.ts`:
- Around line 5-9: Replace the global JSX namespace augmentation with a Solid.js
module augmentation: remove the top-level `namespace JSX { interface
IntrinsicElements { "hang-watch-ui": HTMLElement; } }` and instead declare
`declare module "solid-js" { namespace JSX { interface IntrinsicElements {
"hang-watch-ui": JSX.IntrinsicElements["div"]; } } }` (or use
`JSX.IntrinsicElements["div"]`/appropriate `HTMLAttributes` type) so the custom
element `"hang-watch-ui"` is typed as element props/attributes in Solid.js
rather than as an `HTMLElement`.
🧹 Nitpick comments (1)
js/hang/src/publish/element.ts (1)
248-254: Global JSXIntrinsicElementsomits standard HTML attributes.The non-React JSX declaration only includes the
Observedproperties. Consumers using frameworks that rely on the globalJSXnamespace (e.g., Preact without React compat) won't get type-checking or autocompletion for standard HTML attributes likeclass,id,style, etc. This may be intentional, but worth confirming.
js/hang-ui/src/watch/watch.d.ts
Outdated
| namespace JSX { | ||
| interface IntrinsicElements { | ||
| "hang-watch-ui": HTMLElement; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the sibling publish.d.ts file for JSX typing pattern
find . -name 'publish.d.ts' -type f | head -1 | xargs catRepository: moq-dev/moq
Length of output: 460
🏁 Script executed:
#!/bin/bash
# Search for Solid.js JSX type imports and usage
rg -n 'from.*solid|import.*solid' --type ts -A 2 | head -30Repository: moq-dev/moq
Length of output: 2298
🏁 Script executed:
#!/bin/bash
# Check the watch.d.ts file content to see full context
cat -n js/hang-ui/src/watch/watch.d.tsRepository: moq-dev/moq
Length of output: 615
🏁 Script executed:
#!/bin/bash
# Search for JSX.HTMLAttributes usage in the codebase
rg 'JSX\.HTMLAttributes' --type tsRepository: moq-dev/moq
Length of output: 37
🏁 Script executed:
#!/bin/bash
# Check how JSX is declared/imported in component files
rg -A 5 'type JSX' js/hang-ui/src --type ts | head -40Repository: moq-dev/moq
Length of output: 1784
🌐 Web query:
Solid.js JSX.IntrinsicElements custom element web component type declaration best practices
💡 Result:
Best practices for typing custom elements in Solid.js (JSX.IntrinsicElements)
1) Augment solid-js, not the global JSX namespace
Solid keeps its JSX types in the solid-js module, so add your custom elements via module augmentation (typically in src/types/custom-elements.d.ts, included by tsconfig.json). This pattern is also what Solid maintainers point to for custom elements. [1]
// src/types/custom-elements.d.ts
import type { JSX } from "solid-js";
declare module "solid-js" {
namespace JSX {
interface IntrinsicElements {
"my-widget": JSX.HTMLAttributes<MyWidgetElement> & {
// attributes (stringy / reflected)
"attr:title"?: string;
// properties (typed, not string-coerced)
"prop:value"?: number;
// boolean presence attributes
"bool:disabled"?: boolean;
// custom events (Solid recommends `on:` for native/custom events)
"on:change"?: (e: Event) => void;
"on:my-event"?: (e: CustomEvent<{ id: string }>) => void;
};
}
}
}
declare global {
interface MyWidgetElement extends HTMLElement {
value: number;
}
interface HTMLElementTagNameMap {
"my-widget": MyWidgetElement;
}
}Why this works: TypeScript checks intrinsic JSX tags by looking them up on JSX.IntrinsicElements, and interfaces merge naturally via declaration merging. [2]
2) Model Solid’s web-component-oriented prefixes in your types
When consuming web components, Solid commonly uses:
attr:*to force attribute setting (useful for custom elements) [3]prop:*to force property setting [4]bool:*for boolean presence attributes (especially common for custom elements) [5]on:*for native/custom event listeners [6]
If you don’t include these prefixed keys in your element’s prop type, you’ll lose type-safety right where Solid expects you to use it.
3) Prefer codegen from a Custom Elements Manifest if you have many components
If you publish/consume lots of web components, generate typings from a Custom Elements Manifest and then extend Solid’s IntrinsicElements with a Solid-specific helper type (so the generated output includes Solid’s attr:/prop:/bool: conventions). One example workflow is to extend IntrinsicElements from a generated CustomElementsSolidJs type. [7]
Sources
- Solid discussion showing
declare module "solid-js" { namespace JSX { interface IntrinsicElements { ... }}}for custom elements (github.com) - TypeScript handbook: intrinsic elements are validated via
JSX.IntrinsicElements(+ declaration merging behavior) (typescriptlang.org) - Solid
attr:*docs (docs.solidjs.com) - Solid
prop:*docs (docs.solidjs.com) - Solid
bool:*docs (docs.solidjs.com) - Solid event handler docs (
on:for native/custom events) (docs.solidjs.com) - Example of Solid-specific generated type (
CustomElementsSolidJs) approach (wc-toolkit.com)
Citations:
- 1: How to extend solid JSX.IntrinsicElements namespace for custom web components? solidjs/solid#1123
- 2: https://www.typescriptlang.org/docs/handbook/jsx?utm_source=openai
- 3: https://docs.solidjs.com/reference/jsx-attributes/attr?utm_source=openai
- 4: https://docs.solidjs.com/reference/jsx-attributes/prop?utm_source=openai
- 5: https://docs.solidjs.com/reference/jsx-attributes/bool?utm_source=openai
- 6: https://docs.solidjs.com/concepts/components/event-handlers?utm_source=openai
- 7: https://wc-toolkit.com/integrations/jsx/?utm_source=openai
Use Solid.js module augmentation for custom element types instead of the global JSX namespace.
The current global JSX.IntrinsicElements declaration uses HTMLElement as the value type, which is semantically incorrect—intrinsic elements should describe the props/attributes the component accepts, not the element instance type. More importantly, for Solid.js projects, the correct pattern is to augment the solid-js module directly, not the global JSX namespace.
The current approach works for React (via the React module augmentation override) but is not idiomatic for Solid.js and may cause type-checking issues in Solid contexts.
Recommended fix
Replace the global JSX namespace with a Solid.js module augmentation. For example:
declare global {
interface HTMLElementTagNameMap {
"hang-watch-ui": HTMLElement;
}
- namespace JSX {
- interface IntrinsicElements {
- "hang-watch-ui": HTMLElement;
- }
- }
}
+declare module "solid-js" {
+ namespace JSX {
+ interface IntrinsicElements {
+ "hang-watch-ui": JSX.HTMLAttributes<HTMLElement>;
+ }
+ }
+}This ensures Solid.js uses the correct attribute types and enables full type safety including Solid-specific attribute prefixes (attr:, prop:, bool:, on:).
🤖 Prompt for AI Agents
In `@js/hang-ui/src/watch/watch.d.ts` around lines 5 - 9, Replace the global JSX
namespace augmentation with a Solid.js module augmentation: remove the top-level
`namespace JSX { interface IntrinsicElements { "hang-watch-ui": HTMLElement; }
}` and instead declare `declare module "solid-js" { namespace JSX { interface
IntrinsicElements { "hang-watch-ui": JSX.IntrinsicElements["div"]; } } }` (or
use `JSX.IntrinsicElements["div"]`/appropriate `HTMLAttributes` type) so the
custom element `"hang-watch-ui"` is typed as element props/attributes in
Solid.js rather than as an `HTMLElement`.
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
js/hang-ui/package.json (1)
43-43:⚠️ Potential issue | 🟡 MinorRemove the unused
solid-elementdependency fromjs/hang-ui/package.json.The migration from
solid-element'scustomElement()to nativeHTMLElementsubclasses inpublish/index.tsxandwatch/index.tsxis complete. No files in hang-ui import fromsolid-element, so this dependency can be removed.
🤖 Fix all issues with AI agents
In `@js/hang-ui/package.json`:
- Line 27: Update the prebuild script in package.json to use bun's runner
instead of npx: replace the "prebuild" script that currently invokes "npx cem
analyze --config cem.config.js && bun ../scripts/element-wrappers/index.ts" so
it calls "bunx cem analyze --config cem.config.js && bun
../scripts/element-wrappers/index.ts" (i.e., change the npx invocation to bunx)
to comply with js/** project guidelines; modify the "prebuild" entry in
package.json accordingly.
- Around line 9-20: The rewritePath() function in scripts/package.ts is
incorrectly treating emitted declaration files as source files and turning .d.ts
into .d.js; update rewritePath to first detect and return declaration filenames
unchanged (e.g., if path matches /\.d\.tsx?$/ or endsWith('.d.ts') return the
original path), and only apply the source-file transformation (replace
/\.(ts|tsx)$/ -> '.js') for non-declaration .ts/.tsx inputs so that emitted
types remain as .d.ts in the generated package.json.
In `@js/hang-ui/README.md`:
- Around line 129-138: Add a language identifier to the fenced code block that
shows the directory structure in README.md (the block containing
"element-wrappers/" and its tree) so it renders correctly and satisfies linting;
update the opening fence from ``` to ```text (or another appropriate language
tag) for that directory listing block.
In `@js/hang/package.json`:
- Line 28: The prebuild script in package.json uses npx ("prebuild": "npx cem
analyze --config cem.config.js && bun ../scripts/element-wrappers/index.ts"),
but project guidelines require bunx/bun for JS/TS script execution; update the
prebuild script to call bunx for the cem analyze invocation (and ensure any
subsequent bun invocation remains valid) so it uses bunx cem analyze --config
cem.config.js && bun ../scripts/element-wrappers/index.ts, keeping the rest of
the script intact.
- Around line 17-20: The types path in package.json
("./wrappers/react/index.d.ts") is being mis-transformed by rewritePath()
because the regex /\.ts(x)?$/ converts ".d.ts" to ".d.js"; fix by updating the
types field to "./src/wrappers/react/index.d.ts" so it matches the ./src prefix
and rewrites correctly to "./wrappers/react/index.d.ts", or alternatively change
the rewritePath function to skip files ending in ".d.ts" (i.e., ensure
rewritePath's regex excludes ".d.ts" before applying the .ts/.tsx -> .js
rewrite). Ensure you update the "types" entry in package.json or adjust
rewritePath accordingly (referencing the types field and the rewritePath
function).
In `@js/scripts/element-wrappers/generators/react.ts`:
- Around line 27-43: The generated TypeScript interface in the attrs
construction maps element.attributes to bare identifiers, which breaks on
hyphenated names; update the mapping in the element.attributes -> attrs code
path to emit quoted property names using attr.name (e.g., produce a string-keyed
property like 'attr.name'?: type) so hyphenated or reserved names become valid
TypeScript property literals; ensure proper escaping of any inner quotes when
building the string for attrs.
In `@js/scripts/element-wrappers/README.md`:
- Around line 24-26: Update the README examples to use bun instead of npx for
running scripts: replace the command string "npx cem analyze --config
cem.config.js" with the equivalent "bun run cem analyze --config cem.config.js"
(and likewise update the prebuild example around the other command on line ~80)
so all js/** script examples follow the project guideline to use bun for
TypeScript/JavaScript package management and script execution.
🧹 Nitpick comments (6)
js/scripts/element-wrappers/index.ts (1)
25-56:generateWrappersisasyncbut neverawaits — errors will be double-logged.Two observations:
The function is marked
asyncbut contains noawait. Since all generators are synchronous (perGeneratorFunctiontype on line 15),asyncis unnecessary.On generator failure, the inner
catch(line 42) logs and rethrows, then the outercatch(line 51) logs again. Every failure produces two error messages (❌ React failed: ...+❌ Generation failed: ...).If the intent is to support async generators in the future, update
GeneratorFunctionto returnvoid | Promise<void>andawaitthe call. Otherwise, dropasync.Suggested fix (keep sync)
-async function generateWrappers(): Promise<void> { +function generateWrappers(): void {js/scripts/element-wrappers/generators/react.ts (1)
112-123: Considerimport * as Reactand dropping the timestamp.Two minor points on the generated header:
import React from "react"(default import) can fail in environments with strict ESM/CJS interop.import * as React from "react"is the safer pattern for code that explicitly referencesReact.forwardRef,React.createElement, etc.The embedded timestamp means the generated file changes on every run even when nothing else differs, producing noisy diffs and unnecessary churn in version control (even if
.gitignored, CI caches can be affected).Suggested fix
-function generateFileHeader(): string { - const timestamp = new Date().toISOString(); - return `/** +function generateFileHeader(): string { + return `/** * Auto-generated React wrappers for custom elements * DO NOT EDIT MANUALLY - Generated from custom-elements.json - * - * Generated: ${timestamp} */ -import React from "react"; +import * as React from "react"; `; }js/hang-ui/src/watch/index.tsx (1)
52-66:hang-watchchild lookup is a one-time snapshot.
this.querySelector("hang-watch")runs once inside the microtask. If the child element is added later (e.g., by a framework that renders asynchronously or lazily),WatchUIwill never mount. ThequeueMicrotaskdeferral helps for synchronous framework hydration, but won't cover truly async child insertion.If this is intentional, a brief comment documenting the expectation would help future maintainers. If late-arriving children should be supported, a
MutationObserveron the light DOM could re-trigger rendering.js/scripts/element-wrappers/utils/manifest.ts (3)
61-74: Variablecontentshadows the outer function parameter.Line 63 declares
let contentwhich shadows thecontentparameter on line 27. This is confusing and would likely be flagged by Biome'snoShadowRestrictedNames/noParameterAssignrules. Rename to something likeexampleContent.♻️ Proposed fix
for (const exampleMatch of exampleMatches) { const label = exampleMatch[1]; - let content = exampleMatch[2]; + let exampleContent = exampleMatch[2]; // Remove JSDoc asterisks and clean up formatting - content = content + exampleContent = exampleContent .split("\n") .map((line) => line.replace(/^\s*\*\s?/, "")) .join("\n") .trim(); - if (content) { - examples[label] = content; + if (exampleContent) { + examples[label] = exampleContent; } }
221-232: Comment/code mismatch: "ui" is uppercased at any position, not just the last part; "api" is documented but not handled.The JSDoc on line 212 mentions
"api"as an example abbreviation, and the inline comment on line 225 says "if it's the last part", but the code uppercases"ui"at any position and doesn't handle"api"at all. Either update the comments to match the actual behavior or extend the logic if other abbreviations are intended.♻️ Suggestion: use an abbreviation set for consistency
+const UPPERCASE_PARTS = new Set(["ui", "api"]); + export function tagNameToComponentName(tagName: string): string { return tagName .split("-") .map((part) => { - // Keep "ui" uppercase if it's the last part - if (part.toLowerCase() === "ui") { - return "UI"; + // Keep known abbreviations fully uppercase + if (UPPERCASE_PARTS.has(part.toLowerCase())) { + return part.toUpperCase(); } return part.charAt(0).toUpperCase() + part.slice(1); }) .join(""); }
114-134: Consider usingreaddirSyncwith{ withFileTypes: true }to avoid extrastatSynccalls.The recursive scan calls
statSyncon every entry. Using{ withFileTypes: true }eliminates the extra syscall per entry.♻️ Proposed fix
const scanDirectory = (dir: string): void => { - const entries = readdirSync(dir); + const entries = readdirSync(dir, { withFileTypes: true }); for (const entry of entries) { - const fullPath = join(dir, entry); - const stat = statSync(fullPath); + const fullPath = join(dir, entry.name); - if (stat.isDirectory()) { + if (entry.isDirectory()) { scanDirectory(fullPath); - } else if (entry.endsWith(".ts") || entry.endsWith(".tsx")) { + } else if (entry.name.endsWith(".ts") || entry.name.endsWith(".tsx")) { const source = readFileSync(fullPath, "utf-8");
| "./publish": { | ||
| "types": "./publish/index.d.ts", | ||
| "default": "./src/publish/index.tsx" | ||
| }, | ||
| "./watch": { | ||
| "types": "./watch/index.d.ts", | ||
| "default": "./src/watch/index.tsx" | ||
| }, | ||
| "./react": { | ||
| "types": "./wrappers/react/index.d.ts", | ||
| "default": "./src/wrappers/react/index.ts" | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check vite config for DTS plugin output settings
fd -t f 'vite.config' js/hang-ui/ --exec cat {}Repository: moq-dev/moq
Length of output: 824
🏁 Script executed:
#!/bin/bash
# Check tsconfig for declaration output paths
fd -t f 'tsconfig*.json' js/hang-ui/ --exec cat {}Repository: moq-dev/moq
Length of output: 258
🏁 Script executed:
#!/bin/bash
# Also check the actual package.json to see full context
cat js/hang-ui/package.jsonRepository: moq-dev/moq
Length of output: 1518
🏁 Script executed:
#!/bin/bash
# Check if the package.ts script modifies the exports
fd -t f 'package.ts' js/scripts/ --exec cat {}Repository: moq-dev/moq
Length of output: 2598
Fix the type declaration path transformation in the build script.
The vite-plugin-dts correctly outputs types to dist/ with .d.ts extensions (e.g., dist/publish/index.d.ts). However, the scripts/package.ts build script incorrectly transforms these paths: the rewritePath() function matches .d.ts with its .ts(x)?$ regex and converts it to .d.js. This causes ./publish/index.d.ts to become ./publish/index.d.js in the generated dist/package.json, while the actual files remain as .d.ts. Consumers will fail to resolve types.
Update scripts/package.ts to preserve .d.ts extensions—only .ts/.tsx sources (not declaration files) should convert to .js.
🤖 Prompt for AI Agents
In `@js/hang-ui/package.json` around lines 9 - 20, The rewritePath() function in
scripts/package.ts is incorrectly treating emitted declaration files as source
files and turning .d.ts into .d.js; update rewritePath to first detect and
return declaration filenames unchanged (e.g., if path matches /\.d\.tsx?$/ or
endsWith('.d.ts') return the original path), and only apply the source-file
transformation (replace /\.(ts|tsx)$/ -> '.js') for non-declaration .ts/.tsx
inputs so that emitted types remain as .d.ts in the generated package.json.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
js/hang-ui/README.md
Outdated
| ``` | ||
| element-wrappers/ | ||
| ├── index.ts # Orchestrator - runs all enabled generators | ||
| ├── generators/ | ||
| │ └── react.ts # React wrapper generator | ||
| └── utils/ | ||
| ├── manifest.ts # CEM loader + JSDoc extraction | ||
| ├── codegen.ts # JSDoc + code formatting | ||
| └── types.ts # TypeScript interfaces | ||
| ``` |
There was a problem hiding this comment.
Add language specification to fenced code block.
The directory structure code block should specify a language identifier for proper rendering and to satisfy linting rules.
📝 Proposed fix
-```
+```text
element-wrappers/
├── index.ts # Orchestrator - runs all enabled generators
├── generators/📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ``` | |
| element-wrappers/ | |
| ├── index.ts # Orchestrator - runs all enabled generators | |
| ├── generators/ | |
| │ └── react.ts # React wrapper generator | |
| └── utils/ | |
| ├── manifest.ts # CEM loader + JSDoc extraction | |
| ├── codegen.ts # JSDoc + code formatting | |
| └── types.ts # TypeScript interfaces | |
| ``` |
🧰 Tools
🪛 markdownlint-cli2 (0.20.0)
[warning] 129-129: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
In `@js/hang-ui/README.md` around lines 129 - 138, Add a language identifier to
the fenced code block that shows the directory structure in README.md (the block
containing "element-wrappers/" and its tree) so it renders correctly and
satisfies linting; update the opening fence from ``` to ```text (or another
appropriate language tag) for that directory listing block.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@js/scripts/element-wrappers/README.md`:
- Around line 9-18: The README's directory-tree fenced code block is missing a
language identifier causing MD040; edit the README.md where the block showing
"element-wrappers/ ├── index.ts ..." is defined and change the opening fence
from ``` to ```text (i.e., add the "text" language specifier) so the tree
diagram is treated as plain text. Ensure the closing fence remains ``` and do
not modify the content of the tree (files like index.ts, generators/react.ts,
utils/manifest.ts, codegen.ts, types.ts) aside from the fence change.
🧹 Nitpick comments (1)
js/scripts/element-wrappers/generators/react.ts (1)
121-121: Prefer namespace import for React in generated ESM output.
import React from "react"relies onesModuleInterop/ synthetic default exports. In strict ESM environments (which"type": "module"packages may encounter), this can fail because React's main export is a CommonJS module without a true default export.import * as React from "react"is the safer choice for generated code that must work across different bundler and runtime configurations.Suggested fix
-import React from "react"; +import * as React from "react";
| ``` | ||
| element-wrappers/ | ||
| ├── index.ts # Orchestrator — runs all enabled generators | ||
| ├── generators/ | ||
| │ └── react.ts # React wrapper generator (forwardRef + JSX types) | ||
| └── utils/ | ||
| ├── manifest.ts # CEM loader + JSDoc metadata extraction | ||
| ├── codegen.ts # JSDoc generation + code formatting | ||
| └── types.ts # TypeScript interfaces for CEM structures | ||
| ``` |
There was a problem hiding this comment.
Add language identifier to the fenced code block.
The directory-tree code block at Line 9 is missing a language specifier, which triggers the MD040 lint warning. Use ```text for plain-text/tree diagrams.
Suggested fix
-```
+```text
element-wrappers/🧰 Tools
🪛 markdownlint-cli2 (0.20.0)
[warning] 9-9: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
In `@js/scripts/element-wrappers/README.md` around lines 9 - 18, The README's
directory-tree fenced code block is missing a language identifier causing MD040;
edit the README.md where the block showing "element-wrappers/ ├── index.ts ..."
is defined and change the opening fence from ``` to ```text (i.e., add the
"text" language specifier) so the tree diagram is treated as plain text. Ensure
the closing fence remains ``` and do not modify the content of the tree (files
like index.ts, generators/react.ts, utils/manifest.ts, codegen.ts, types.ts)
aside from the fence change.
jdreetz
left a comment
jdreetz
left a comment
There was a problem hiding this comment.
Seems good to me. haven't tested. left a few questions though.
| @@ -1,27 +1,74 @@ | |||
| import type HangPublish from "@moq/hang/publish/element"; | |||
| import { customElement } from "solid-element"; | |||
There was a problem hiding this comment.
Do we have to get rid of solid-element to support CEM? I thought it just needs the more detailed JS Docs comment? Although we're not making much use of it currently, solid-element makes handling setters and getters a lot easier than the vanilla Custom Element syntax, so I'd prefer to keep it if possible.
There was a problem hiding this comment.
You're right, keeping solid-element for getters/setters makes sense. I'll revert those changes and focus on fixing the JSDoc comments for CEM support instead.
| @@ -1,26 +1,77 @@ | |||
| import type HangWatch from "@moq/hang/watch/element"; | |||
| import { customElement } from "solid-element"; | |||
There was a problem hiding this comment.
same question as above re: removing solid-element.
| onMount(async () => { | ||
| await customElements.whenDefined("hang-watch"); | ||
| const watchEl = element.querySelector("hang-watch"); | ||
| setNested(watchEl ? (watchEl as HangWatch) : undefined); | ||
| }); |
There was a problem hiding this comment.
we don't need any of this anymore because of the queueMicrotask stuff below?
| "./publish": { | ||
| "types": "./publish/index.d.ts", | ||
| "default": "./src/publish/index.tsx" | ||
| }, | ||
| "./watch": { | ||
| "types": "./watch/index.d.ts", | ||
| "default": "./src/watch/index.tsx" | ||
| }, | ||
| "./react": { | ||
| "types": "./wrappers/react/index.d.ts", | ||
| "default": "./src/wrappers/react/index.ts" | ||
| } |
There was a problem hiding this comment.
I think it's really cool we're adding support for all this, I'm just surprised there isn't another library that can replace all this transformation / code gen code. Maybe that's all coming in the future as people standardize on the methods though?
There was a problem hiding this comment.
You're right, there are some NPM packages available, but they didn't really fit what I had in mind for our specific needs. Anyway, I’m waiting for your PR (#948) to be merged before I introduce the CEM support, since the structure has changed now. So, I'm going to close this PR for now and I'll open a fresh one once yours is merged. What do you think? Thank you!
There was a problem hiding this comment.
Yeah I think this is too much. A react wrapper would be cool but this feels finicky and a maintenance overhead.
Maybe start with an explicit react component? If it's too much duplication, then we can venture into code generation.
There was a problem hiding this comment.
The React wrapper and custom element types seem valuable to me, but yea, the code generation stuff feels like it might be too much at this point.
| // Auto-generated by scripts/generate-wrappers.ts during prebuild phase | ||
| "wrappers/react/index": resolve(__dirname, "src/wrappers/react/index.ts"), |
There was a problem hiding this comment.
If a user adds another wrapper, will it get added here automatically during pre-build?
| * @attr {string} url - The WebTransport URL of the MOQ relay server (e.g., "https://relay.example.com") | ||
| * @attr {string} path - The broadcast path to subscribe to (e.g., "/live/stream") | ||
| * @attr {boolean} paused - Whether playback is paused | ||
| * @attr {number} volume - Audio volume level (0.0 to 1.0, default: 0.5) | ||
| * @attr {boolean} muted - Whether audio is muted | ||
| * @attr {boolean} reload - Whether to automatically reconnect on connection loss | ||
| * @attr {number} jitter - Target jitter buffer in milliseconds (default: 100) |
There was a problem hiding this comment.
Are these types correct? Aren't they returning primitive wrapped Signals?
| function formatDocSection( | ||
| title: string, | ||
| items: Array<{ name: string; description?: string; extra?: string }>, | ||
| ): string[] { |
There was a problem hiding this comment.
can we type this any more specifically?
| } | ||
|
|
||
| /** | ||
| * Generate JSDoc comment block from custom element metadata |
There was a problem hiding this comment.
this is generating CEM specific JSDoc comments, isn't it? Should we mention that?
There was a problem hiding this comment.
I'm surprised the functionality in this file isn't available in a library already yet, but maybe it's too new.
| set path(value: string | Moq.Path.Valid | undefined) { | ||
| value ? this.setAttribute("path", String(value)) : this.removeAttribute("path"); | ||
| } |
There was a problem hiding this comment.
Currently, the usage pattern is:
watch.path.set("foo")
watch.path.get()
watch.path.changed(() => { ... })And you can use the React helper:
import { react } from "@moq/signals"
const path = react(watch.path);This PR would change it to:
watch.path = "foo"
watch.path.get()
watch.path.set("foo") // still works
watch.path.changed(() => { ... })I don't like the inconsistency there; I'd rather have an explicit .set.
There was a problem hiding this comment.
Yeah I think this is too much. A react wrapper would be cool but this feels finicky and a maintenance overhead.
Maybe start with an explicit react component? If it's too much duplication, then we can venture into code generation.
@pzanella if it's easier to open a new one, go for it, but I'd make sure to address the comments if you can. |
3 participants
This PR makes
@moq/hangand@moq/hang-uiweb components work better with React.url,path,jitter. This allows React to pass data directly through props