Repository navigation
feat(props): interpolate placeholders in bound props - #490
Open
adamdehaven wants to merge 1 commit into
Open
adamdehaven wants to merge 1 commit into
adamdehaven wants to merge 1 commit into
Conversation
Resolve placeholders such as path or default in colon-prefixed string props at render time. They read the existing render context: frontmatter, meta, data and props. The result is always a string, and no plugin is needed. Only text, numbers and booleans are inserted. A missing value uses the default. A prop that is a single placeholder with nothing to show is omitted. Malformed placeholders, unknown roots, JSON values and the key, ref, is, as and style props keep their current behavior. The final text is checked for unsafe URL schemes and unsafe prop names with the existing validation. Also fix a scope leak. An element whose props were all omitted inherited its parent props in the HTML renderer, and its grandparent props in the Vue, React, Svelte and Angular child scope. Ported from the nuxtjs/mdc implementation: nuxt-content/mdc#516 Closes comarkdown#489
|
1 finding · reviewed 🟠
|
|
@adamdehaven is attempting to deploy a commit to the NuxtLabs Team on Vercel. A member of the Team first needs to authorize it. |
Contributor
Documentation previewsPreviews are disabled for pull requests from forks. |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 52.94% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 17 functions across 22 files. (7 skipped:… | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly and concisely describes the main change: adding placeholder interpolation to bound props. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 52.94% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 17 functions across 22 files. (7 skipped: 7 unsupported.)
- Fix all pre-merge checks with AI
✨ Finishing Touches: 💡 1
-
🛠️ Fix failing CI checks: 💡
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 help to get the list of available commands.
comark
@comark/angular
@comark/ansi
@comark/html
@comark/nuxt
@comark/react
@comark/svelte
@comark/vue
commit: |
Contributor
📦 Bundle size snapshot is out of dateThe published size of one or more packages changed, so the inline snapshot in Please review the diff below. If the change is expected, accept the new snapshot:
Only maintainers with write access can trigger the update. You can also comment Bundle size diff FAIL test/bundle.test.ts > package bundle size > published size of each package
Error: Snapshot `package bundle size > published size of each package 1` mismatched
- Expected
+ Received
{
- "@comark/angular": "56.2k (72 files)",
+ "@comark/angular": "56.5k (72 files)",
"@comark/ansi": "37.3k (98 files)",
"@comark/html": "16.5k (58 files)",
"@comark/nuxt": "11.9k (58 files)",
- "@comark/react": "38.0k (76 files)",
+ "@comark/react": "38.1k (76 files)",
- "@comark/svelte": "45.8k (84 files)",
+ "@comark/svelte": "46.0k (84 files)",
- "@comark/vue": "56.2k (80 files)",
+ "@comark/vue": "56.4k (80 files)",
- "comark": "384k (152 files)",
+ "comark": "389k (154 files)",
}
❯ test/bundle.test.ts:61:20
59| }
60|
61| expect(report).toMatchInlineSnapshot(`
| ^
62| {
63| "@comark/angular": "56.2k (72 files)",
⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯[1/1]⎯ |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Adds
{{ path || default }}interpolation inside:props, such as:href="/docs/{{ data.slug }}/overview".It is resolved at render time against the existing
{ frontmatter, meta, data, props }context and always produces a string.It also fixes a scope bug where an element whose props were all omitted inherited its parent's (HTML) or grandparent's (Vue, React, Svelte, Angular)
props.Why
Authors could only bind a whole data path, so a link or label could not be built from fixed text plus data (#489). This ports the approach from nuxt-content/mdc#516, adapted to Comark's shared resolver. The scope fix is included because omitting unresolved props made that existing leak easy to hit.
Alternatives considered:
:bindings already work without it.get()is reused instead.validateProp, the same validators static values get.:count="data.n"), so the result is always a string.Notes
:,key,ref,is,asandstyle, malformed placeholders, unknown roots and JSON values.:prop that already contained a well-formed{{ data.x }}now interpolates.security()allowlists apply to literal values only.Walkthrough
Interpolation helper
internal/stringify/interpolate.tsscans the value withindexOf, parses placeholders and inserts text, numbers and booleans in a single pass. It caps output at 4096 characters, cleans control characters and runsvalidatePropon the final text. It returns the string,UNRESOLVED(omit the prop) orREJECTED_PROP, orundefinedto keep today's behavior.Shared resolver hook
resolveAttributes(both modes) andresolveAttributecall the helper inattributes.ts. Vue, React, Svelte, Angular, HTML and ANSI get the feature with no renderer changes.Child scope fix
state.tsand the Vue, React, Svelte and Angular scope code now decide whether to shadowpropsfrom the raw attributes, not from the resolved ones.Tests
resolveAttributes, and DoS shapes within the length limit.Docs and examples
dev:bindingand the docs playground.Summary by CodeRabbit