Skip to content

Patch added to handle the reactive classes or dynamic classes handlin… #2678

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.

Sign up for GitHub

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

Open
khathija-ahamadi wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Patch added to handle the reactive classes or dynamic classes handlin… #2678

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
60 changes: 60 additions & 0 deletions patches/@stencil__vue-output-target.patch
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
diff --git a/dist/runtime.cjs.js b/dist/runtime.cjs.js
index 8d1b57d4d3581b3681075b35380659f969469f35..29c3763e94e90ce26043aae72ac927eeef91881c 100644
--- a/dist/runtime.cjs.js
+++ b/dist/runtime.cjs.js
@@ -119,7 +119,7 @@ const ARIA_PROP_PREFIX = 'aria';
const EMPTY_PROP = Symbol();
const DEFAULT_EMPTY_PROP = { default: EMPTY_PROP };
const getComponentClasses = (classes) => {
- return classes?.split(' ') || [];
+ return (classes?.split(' ') || []).filter(Boolean);
Comment on lines +8 to +10

@coderabbitai coderabbitai Bot Jul 30, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Add regression coverage for class ownership and removal.

This changes runtime behavior in both bundles, but no regression test is included. Cover reactive class removal, preservation of externally/component-added ix-invalid, and empty class bindings.

As per path instructions: “Prioritize correctness, regressions, accessibility, release impact, and missing validation.”

Also applies to: 20-27, 38-40, 50-57

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@patches/`@stencil__vue-output-target.patch around lines 8 - 10, Add
regression tests around getComponentClasses and the affected runtime bundles
covering reactive class removal, preservation of externally or component-added
ix-invalid, and empty class bindings. Verify classes are correctly owned and
removed without deleting unrelated classes, while preserving existing behavior
for valid class strings.

Source: Path instructions

@khathija-ahamadi khathija-ahamadi Jul 30, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nuke-ellington
I will add tests once initial review is done and go ahead with the solution is given.

Manual tests in all frameworks is carried out and the solution is working as expected.

};
const syncElementClasses = (ref, componentClasses, defaultClasses = []) => {
if (ref?.value) {
@@ -258,9 +258,14 @@ const defineContainer = (name, defineCustomElement, componentProps = [], emitPro
};
return () => {
modelPropValue = props[modelProp];
- getComponentClasses(attrs.class).forEach((value) => {
- classes.add(value);
+ const nextClasses = new Set(getComponentClasses(attrs.class));
+ classes.forEach((value) => {
+ if (!nextClasses.has(value)) {
+ containerRef.value?.classList.remove(value);
+ }
});
+ classes.clear();
+ nextClasses.forEach((value) => classes.add(value));
// @ts-expect-error
const oldClick = props.onClick;
const handleClick = (ev) => {
diff --git a/dist/runtime.js b/dist/runtime.js
index 7efe4d9bba43a0533f2d3f6d17d5ed6f816ee04e..b483fcbde01193a76869b9a2c96b48685ad9db92 100644
--- a/dist/runtime.js
+++ b/dist/runtime.js
@@ -117,7 +117,7 @@ const ARIA_PROP_PREFIX = 'aria';
const EMPTY_PROP = Symbol();
const DEFAULT_EMPTY_PROP = { default: EMPTY_PROP };
const getComponentClasses = (classes) => {
- return classes?.split(' ') || [];
+ return (classes?.split(' ') || []).filter(Boolean);
};
const syncElementClasses = (ref, componentClasses, defaultClasses = []) => {
if (ref?.value) {
@@ -256,9 +256,14 @@ const defineContainer = (name, defineCustomElement, componentProps = [], emitPro
};
return () => {
modelPropValue = props[modelProp];
- getComponentClasses(attrs.class).forEach((value) => {
- classes.add(value);
+ const nextClasses = new Set(getComponentClasses(attrs.class));
+ classes.forEach((value) => {
+ if (!nextClasses.has(value)) {
+ containerRef.value?.classList.remove(value);
+ }
});
+ classes.clear();
+ nextClasses.forEach((value) => classes.add(value));
// @ts-expect-error
const oldClick = props.onClick;
const handleClick = (ev) => {
13 changes: 12 additions & 1 deletion patches/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,15 @@
# Stencil Core Patch
# Stencil Patches

## Vue Output Target
Comment on lines +1 to +3

@coderabbitai coderabbitai Bot Jul 30, 2026

Copy link
Copy Markdown

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

Use a conventional PR title.

The current title is a prose sentence rather than <type>[optional scope]: <description>; use a title such as fix(vue): patch reactive class synchronization. (github.com)

As per path instructions: “Expect title format: [optional ]: .”

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@patches/README.md` around lines 1 - 3, Update the pull request title to
follow the required conventional format, using a type, optional scope, colon,
and concise description; for example, use a format like “fix(vue): patch
reactive class synchronization.”

Sources: Path instructions, MCP tools


`@stencil__vue-output-target.patch` reconciles reactive Vue class bindings in
the generated component runtime. Without the patch, classes are added when a
binding becomes active but are not removed when the binding becomes inactive.

The patch updates both the ESM and CommonJS runtime bundles and should be
removed after the fix is available in an upstream release.

## Stencil Core

## Why This Patch Exists

Expand Down
11 changes: 7 additions & 4 deletions pnpm-lock.yaml
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions pnpm-workspace.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,3 +25,4 @@ overrides:

patchedDependencies:
'@stencil/core': "patches/@stencil__core.patch"
'@stencil/vue-output-target': "patches/@stencil__vue-output-target.patch"

@coderabbitai coderabbitai Bot Jul 30, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win

Add the required changeset for this consumer-visible fix.

This patch changes shipped Vue behavior, but no changeset is included. Add one for the package that ships the Vue wrappers, or explicitly justify why this is internal-only.

As per path instructions: “Changesets are required for public API updates, behavior changes, styling/theming changes, accessibility changes, and bug fixes with user impact; if no changeset is included, require the author to explicitly justify why the change is internal-only.”

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pnpm-workspace.yaml` at line 28, Add a changeset for the package that ships
the Vue wrappers, describing the consumer-visible behavior fix and selecting the
appropriate release level. If this patch is genuinely internal-only, provide an
explicit justification instead of adding the changeset.

Source: Path instructions

Loading