Skip to content

Commit

Permalink
fix(runtime): SSR class handling breaks normal class handling (#6116)
Browse files Browse the repository at this point in the history
* feat(emit error events): emit custom event on component error within lifecycle or render

* test(add test for component error handling)

* revert un-required changes

* Added host element to loadModule error handling

* chore(format): add prettier

- upgrade prettier to v2.3.2
  - lock version to prevent breaking changes in minor versions
- add prettier.dry-run package.json script
- add pipeline action to evaluate format status
- add prettierignore file for faster runs

STENCIL-8: Add Prettier to Stencil

* format codebase

* revert cherry pick

* feat(emit error events): emit custom event on component error within lifecycle or render

* test(add test for component error handling)

* revert un-required changes

* Added host element to loadModule error handling

* revert cherry pick

* run prettier

* rm rv karma/test-components

* rv extra prettier call

* Flaky test?

* fix lint

* fixup `strictNullChecks` issues

* chore: tidy

* chore: formatting

* chore: revert type

* fix(runtime): SSR handling breaks normal class handling

* chore: typos

* chore: idiot

* chore: add unit test

---------

Co-authored-by: Ryan Waskiewicz <[email protected]>
Co-authored-by: John Jenkins <[email protected]>
  • Loading branch information
3 people authored Jan 25, 2025
1 parent dc4c88f commit 1e8a2d2
Show file tree
Hide file tree
Showing 9 changed files with 292 additions and 21 deletions.
6 changes: 3 additions & 3 deletions src/runtime/vdom/set-accessor.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import { VNODE_FLAGS, XLINK_NS } from '../runtime-constants';
* @param newValue the new value for the attribute
* @param isSvg whether we're in an svg context or not
* @param flags bitflags for Vdom variables
* @param initialRender whether this is the first render of the VDom
*/
export const setAccessor = (
elm: d.RenderNode,
Expand All @@ -36,6 +37,7 @@ export const setAccessor = (
newValue: any,
isSvg: boolean,
flags: number,
initialRender?: boolean,
) => {
if (oldValue !== newValue) {
let isProp = isMemberInElement(elm, memberName);
Expand All @@ -46,17 +48,15 @@ export const setAccessor = (
const oldClasses = parseClassList(oldValue);
let newClasses = parseClassList(newValue);

if (elm['s-si']) {
if (BUILD.hydrateClientSide && elm['s-si'] && initialRender) {
// for `scoped: true` components, new nodes after initial hydration
// from SSR don't have the slotted class added. Let's add that now
newClasses.push(elm['s-si']);
oldClasses.forEach((c) => {
if (c.startsWith(elm['s-si'])) newClasses.push(c);
});
newClasses = [...new Set(newClasses)];

classList.add(...newClasses);
delete elm['s-si'];
} else {
classList.remove(...oldClasses.filter((c) => c && !newClasses.includes(c)));
classList.add(...newClasses.filter((c) => c && !oldClasses.includes(c)));
Expand Down
43 changes: 43 additions & 0 deletions src/runtime/vdom/test/set-accessor.spec.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
import { BUILD } from '@app-data';

import { setAccessor } from '../set-accessor';

describe('setAccessor for custom elements', () => {
Expand Down Expand Up @@ -598,6 +600,10 @@ describe('setAccessor for inputs', () => {
});

describe('setAccessor for standard html elements', () => {
beforeEach(() => {
BUILD.hydrateClientSide = true;
});

describe('simple global attributes', () => {
it('should not add attribute when prop is undefined or null', () => {
const inputElm = document.createElement('section');
Expand Down Expand Up @@ -789,6 +795,43 @@ describe('setAccessor for standard html elements', () => {
setAccessor(elm, 'class', 'md', '', false, 0);
expect(elm.className).toEqual('');
});

it('should add scope classes on initial render if `s-si` set', () => {
const elm = document.createElement('section');

// not s-si set
setAccessor(elm, 'class', 'a-scope-id', undefined, false, 0, true);
expect(elm.className).toEqual('');

(elm as any)['s-si'] = 'a-scope-id';

setAccessor(elm, 'class', '', undefined, false, 0, true);
expect(elm.className).toEqual('a-scope-id');

setAccessor(
elm,
'class',
'a-scope-id-something a-scope-id-something-else unrelated-old-thing',
undefined,
false,
0,
true,
);
expect(elm.className).toEqual('a-scope-id a-scope-id-something a-scope-id-something-else');

elm.className = '';
setAccessor(elm, 'class', 'something-old', 'something-new', false, 0, true);
expect(elm.className).toEqual('something-new a-scope-id');

elm.className = '';
setAccessor(elm, 'class', 'something-old a-scope-id-something', 'something-new', false, 0, true);
expect(elm.className).toEqual('something-new a-scope-id a-scope-id-something');

// just check it reverts to normal behavior after initial render
elm.className = '';
setAccessor(elm, 'class', 'something-old a-scope-id-something', 'something-new', false, 0);
expect(elm.className).toEqual('something-new');
});
});

describe('style attribute', () => {
Expand Down
46 changes: 32 additions & 14 deletions src/runtime/vdom/test/update-element.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -173,28 +173,46 @@ describe('updateElement', () => {
});
updateElement(initialVNode, firstVNode, false);
expect(spy).toHaveBeenCalledTimes(4);
expect(spy).toHaveBeenNthCalledWith(1, elm, 'content', undefined, 'attributes removed', false, 0);
expect(spy).toHaveBeenNthCalledWith(2, elm, 'padding', undefined, false, false, 0);
expect(spy).toHaveBeenNthCalledWith(3, elm, 'bold', undefined, 'false', false, 0);
expect(spy).toHaveBeenNthCalledWith(4, elm, 'no-attr', undefined, null, false, 0);
expect(spy).toHaveBeenNthCalledWith(1, elm, 'content', undefined, 'attributes removed', false, 0, undefined);
expect(spy).toHaveBeenNthCalledWith(2, elm, 'padding', undefined, false, false, 0, undefined);
expect(spy).toHaveBeenNthCalledWith(3, elm, 'bold', undefined, 'false', false, 0, undefined);
expect(spy).toHaveBeenNthCalledWith(4, elm, 'no-attr', undefined, null, false, 0, undefined);
spy.mockReset();

updateElement(firstVNode, secondVNode, false);
expect(spy).toHaveBeenCalledTimes(6);
expect(spy).toHaveBeenNthCalledWith(1, elm, 'content', 'attributes removed', 'attributes added', false, 0);
expect(spy).toHaveBeenNthCalledWith(2, elm, 'padding', false, true, false, 0);
expect(spy).toHaveBeenNthCalledWith(3, elm, 'bold', 'false', 'true', false, 0);
expect(spy).toHaveBeenNthCalledWith(4, elm, 'margin', undefined, '', false, 0);
expect(spy).toHaveBeenNthCalledWith(5, elm, 'color', undefined, 'lime', false, 0);
expect(spy).toHaveBeenNthCalledWith(
1,
elm,
'content',
'attributes removed',
'attributes added',
false,
0,
undefined,
);
expect(spy).toHaveBeenNthCalledWith(2, elm, 'padding', false, true, false, 0, undefined);
expect(spy).toHaveBeenNthCalledWith(3, elm, 'bold', 'false', 'true', false, 0, undefined);
expect(spy).toHaveBeenNthCalledWith(4, elm, 'margin', undefined, '', false, 0, undefined);
expect(spy).toHaveBeenNthCalledWith(5, elm, 'color', undefined, 'lime', false, 0, undefined);
spy.mockReset();

updateElement(secondVNode, firstVNode, false);
expect(spy).toHaveBeenCalledTimes(6);
expect(spy).toHaveBeenNthCalledWith(1, elm, 'margin', '', undefined, false, 0);
expect(spy).toHaveBeenNthCalledWith(2, elm, 'color', 'lime', undefined, false, 0);
expect(spy).toHaveBeenNthCalledWith(3, elm, 'content', 'attributes added', 'attributes removed', false, 0);
expect(spy).toHaveBeenNthCalledWith(4, elm, 'padding', true, false, false, 0);
expect(spy).toHaveBeenNthCalledWith(5, elm, 'bold', 'true', 'false', false, 0);
expect(spy).toHaveBeenNthCalledWith(1, elm, 'margin', '', undefined, false, 0, undefined);
expect(spy).toHaveBeenNthCalledWith(2, elm, 'color', 'lime', undefined, false, 0, undefined);
expect(spy).toHaveBeenNthCalledWith(
3,
elm,
'content',
'attributes added',
'attributes removed',
false,
0,
undefined,
);
expect(spy).toHaveBeenNthCalledWith(4, elm, 'padding', true, false, false, 0, undefined);
expect(spy).toHaveBeenNthCalledWith(5, elm, 'bold', 'true', 'false', false, 0, undefined);
spy.mockReset();
spy.mockRestore();
});
Expand Down
28 changes: 25 additions & 3 deletions src/runtime/vdom/update-element.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,14 @@ import { setAccessor } from './set-accessor';
* @param oldVnode an old virtual DOM node or null
* @param newVnode a new virtual DOM node
* @param isSvgMode whether or not we're in an SVG context
* @param isInitialRender whether this is the first render of the VDOM
*/
export const updateElement = (oldVnode: d.VNode | null, newVnode: d.VNode, isSvgMode: boolean): void => {
export const updateElement = (
oldVnode: d.VNode | null,
newVnode: d.VNode,
isSvgMode: boolean,
isInitialRender?: boolean,
): void => {
// if the element passed in is a shadow root, which is a document fragment
// then we want to be adding attrs/props to the shadow root's "host" element
// if it's not a shadow root, then we add attrs/props to the same element
Expand All @@ -30,14 +36,30 @@ export const updateElement = (oldVnode: d.VNode | null, newVnode: d.VNode, isSvg
// remove attributes no longer present on the vnode by setting them to undefined
for (const memberName of sortedAttrNames(Object.keys(oldVnodeAttrs))) {
if (!(memberName in newVnodeAttrs)) {
setAccessor(elm, memberName, oldVnodeAttrs[memberName], undefined, isSvgMode, newVnode.$flags$);
setAccessor(
elm,
memberName,
oldVnodeAttrs[memberName],
undefined,
isSvgMode,
newVnode.$flags$,
isInitialRender,
);
}
}
}

// add new & update changed attributes
for (const memberName of sortedAttrNames(Object.keys(newVnodeAttrs))) {
setAccessor(elm, memberName, oldVnodeAttrs[memberName], newVnodeAttrs[memberName], isSvgMode, newVnode.$flags$);
setAccessor(
elm,
memberName,
oldVnodeAttrs[memberName],
newVnodeAttrs[memberName],
isSvgMode,
newVnode.$flags$,
isInitialRender,
);
}
};

Expand Down
2 changes: 1 addition & 1 deletion src/runtime/vdom/vdom-render.ts
Original file line number Diff line number Diff line change
Expand Up @@ -684,7 +684,7 @@ export const patch = (oldVNode: d.VNode, newVNode: d.VNode, isInitialRender = fa
// either this is the first render of an element OR it's an update
// AND we already know it's possible it could have changed
// this updates the element's css classes, attrs, props, listeners, etc.
updateElement(oldVNode, newVNode, isSvgMode);
updateElement(oldVNode, newVNode, isSvgMode, isInitialRender);
}
}

Expand Down
28 changes: 28 additions & 0 deletions test/wdio/async-rerender/cmp.test.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
import { h } from '@stencil/core';
import { render } from '@wdio/browser-runner/stencil';
import { $, expect } from '@wdio/globals';

describe('asynchronous re-rendering', () => {
before(async () => {
render({
template: () => <async-rerender></async-rerender>,
});
});

it('button click re-renders', async () => {
await $('async-rerender .number').waitForExist();
const listItems1 = await $$('async-rerender .number');
await expect(listItems1.length).toBe(10);

const button = await $$('button');
await button[0].click();
await $('async-rerender .loaded').waitForExist();
const listItems2 = await $$('async-rerender .number');
await expect(listItems2.length).toBe(5);

await button[1].click();
await $('async-rerender .loaded').waitForExist();
const listItems3 = await $$('async-rerender .number');
await expect(listItems3.length).toBe(10);
});
});
67 changes: 67 additions & 0 deletions test/wdio/async-rerender/cmp.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
import { Component, h, Host, State } from '@stencil/core';

@Component({
tag: 'async-rerender',
scoped: true,
})
export class MyComponent {
@State() data: any[];

@State() isLoaded = false;

componentWillLoad() {
this.fetchData();
}

private asyncThing(): Promise<{ name: string }[]> {
return new Promise((resolve) => {
setTimeout(() => {
const data = Array.from({ length: 20 }, (_, i) => ({ name: `name ${i + 1}` }));

const getRandomItems = (arr: any, num: number) => {
const shuffled = arr.sort(() => 0.5 - Math.random());
return shuffled.slice(0, num);
};

resolve(getRandomItems(data, 10));
}, 500);
});
}

async fetchData() {
this.data = await this.asyncThing();
this.isLoaded = true;
}

async prev() {
this.isLoaded = false;
this.data = (await this.asyncThing()).slice(0, 5);
this.isLoaded = true;
}

async after() {
this.isLoaded = false;
this.data = await this.asyncThing();
this.isLoaded = true;
}

display() {
return this.data !== undefined && this.data !== null;
}

render() {
return (
<Host>
<p>
<button onClick={() => this.prev()}>Previous</button>
<button onClick={() => this.after()}>Next</button>
</p>
{this.display() && (
<section class={`data-state ${this.isLoaded ? 'loaded' : ''}`}>
{this.data?.map((d) => <div class="number">{d.name}</div>)}
</section>
)}
</Host>
);
}
}
45 changes: 45 additions & 0 deletions test/wdio/scoped-add-remove-classes/cmp.test.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
import { h } from '@stencil/core';
import { render } from '@wdio/browser-runner/stencil';
import { $, expect } from '@wdio/globals';

describe('scoped adding and removing of classes', () => {
before(async () => {
render({
template: () => (
<scoped-add-remove-classes
items={[
{ id: 1, label: 'Item 1', selected: false },
{ id: 2, label: 'Item 2', selected: true },
{ id: 3, label: 'Item 3', selected: false },
{ id: 4, label: 'Item 4', selected: false },
{ id: 5, label: 'Item 5', selected: false },
{ id: 6, label: 'Item 6', selected: false },
{ id: 7, label: 'Item 7', selected: false },
{ id: 8, label: 'Item 8', selected: false },
]}
selectedItems={[2]}
></scoped-add-remove-classes>
),
});
});

it('clicking new items, adds class and removes other item classes', async () => {
await $('scoped-add-remove-classes .menu-item').waitForExist();
const items = $$('scoped-add-remove-classes .menu-item');

let selectedItems = await $$('scoped-add-remove-classes .menu-selected');
await expect(selectedItems.length).toBe(1);

await items[0].click();
selectedItems = await $$('scoped-add-remove-classes .menu-selected');
await expect(selectedItems.length).toBe(1);

await items[1].click();
selectedItems = await $$('scoped-add-remove-classes .menu-selected');
await expect(selectedItems.length).toBe(1);

await items[7].click();
selectedItems = await $$('scoped-add-remove-classes .menu-selected');
await expect(selectedItems.length).toBe(1);
});
});
Loading

0 comments on commit 1e8a2d2

Please sign in to comment.