Skip to content

Commit

Permalink
perf: optimize scoped ID for light/native (#4399)
Browse files Browse the repository at this point in the history
  • Loading branch information
nolanlawson authored Jul 25, 2024
1 parent d7147ce commit 750f557
Show file tree
Hide file tree
Showing 49 changed files with 4,605 additions and 90 deletions.
1 change: 1 addition & 0 deletions .github/workflows/karma.yml
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,7 @@ jobs:
- run: API_VERSION=61 yarn sauce:ci
- run: API_VERSION=61 DISABLE_SYNTHETIC=1 yarn sauce:ci
- run: DISABLE_SYNTHETIC_SHADOW_SUPPORT_IN_COMPILER=1 DISABLE_SYNTHETIC=1 yarn sauce:ci
- run: DISABLE_SYNTHETIC_SHADOW_SUPPORT_IN_COMPILER=1 DISABLE_SYNTHETIC=1 DISABLE_STATIC_CONTENT_OPTIMIZATION=1 yarn sauce:ci
- run: ENABLE_ARIA_REFLECTION_GLOBAL_POLYFILL=1 yarn sauce:ci
- run: ENABLE_ARIA_REFLECTION_GLOBAL_POLYFILL=1 DISABLE_SYNTHETIC=1 yarn sauce:ci
- run: DISABLE_STATIC_CONTENT_OPTIMIZATION=1 yarn sauce:ci
Expand Down
2 changes: 1 addition & 1 deletion packages/@lwc/compiler/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ const { code } = transformSync(source, filename, options);
- `customRendererConfig` (type: `object`, optional) - custom renderer config to pass to `@lwc/template-compiler`. See that package's README for details.
- `enableLightningWebSecurityTransforms` (type: `boolean`, default: `false`) - The configuration to enable Lighting Web Security specific transformations.
- `enableLwcSpread` (boolean, optional, `true` by default) - Deprecated. Ignored by compiler. `lwc:spread` is always enabled.
- `disableSyntheticShadowSupport` (type: `boolean`, default: `false`) - Set to true if synthetic shadow DOM support is not needed, which can result in smaller output.
- `disableSyntheticShadowSupport` (type: `boolean`, default: `false`) - Set to true if synthetic shadow DOM support is not needed, which can result in smaller/faster output.
- `instrumentation` (type: `InstrumentationObject`, optional) - instrumentation object to gather metrics and non-error logs for internal use. See the `@lwc/errors` package for details on the interface.
- `apiVersion` (type: `number`, optional) - API version to associate with the compiled module.

Expand Down
2 changes: 1 addition & 1 deletion packages/@lwc/compiler/src/options.ts
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,7 @@ export interface TransformOptions {
customRendererConfig?: CustomRendererConfig;
/** @deprecated Ignored by compiler. `lwc:spread` is always enabled. */
enableLwcSpread?: boolean;
/** Set to true if synthetic shadow DOM support is not needed, which can result in smaller output. */
/** Set to true if synthetic shadow DOM support is not needed, which can result in smaller/faster output. */
disableSyntheticShadowSupport?: boolean;
/**
* Enable transformations specific to {@link https://developer.salesforce.com/docs/platform/lwc/guide/security-lwsec-intro.html Lighting Web Security}.
Expand Down
2 changes: 2 additions & 0 deletions packages/@lwc/compiler/src/transformers/template.ts
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ export default function templateTransform(
namespace,
name,
apiVersion,
disableSyntheticShadowSupport,
} = options;
const experimentalDynamicDirective =
deprecatedDynamicDirective ?? Boolean(experimentalDynamicComponent);
Expand All @@ -62,6 +63,7 @@ export default function templateTransform(
enableDynamicComponents,
instrumentation,
apiVersion,
disableSyntheticShadowSupport,
});
} catch (e) {
throw normalizeToCompilerError(TransformerErrors.HTML_TRANSFORMER_ERROR, e, { filename });
Expand Down
14 changes: 0 additions & 14 deletions packages/@lwc/engine-core/src/framework/api.ts
Original file line number Diff line number Diff line change
Expand Up @@ -564,12 +564,6 @@ function k(compilerKey: number, obj: any): string | void {
function gid(id: string | undefined | null): string | null | undefined {
const vmBeingRendered = getVMBeingRendered()!;
if (isUndefined(id) || id === '') {
if (process.env.NODE_ENV !== 'production') {
logError(
`Invalid id value "${id}". The id attribute must contain a non-empty string.`,
vmBeingRendered
);
}
return id;
}
// We remove attributes when they are assigned a value of null
Expand All @@ -587,14 +581,6 @@ function gid(id: string | undefined | null): string | null | undefined {
function fid(url: string | undefined | null): string | null | undefined {
const vmBeingRendered = getVMBeingRendered()!;
if (isUndefined(url) || url === '') {
if (process.env.NODE_ENV !== 'production') {
if (isUndefined(url)) {
logError(
`Undefined url value for "href" or "xlink:href" attribute. Expected a non-empty string.`,
vmBeingRendered
);
}
}
return url;
}
// We remove attributes when they are assigned a value of null
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,23 +19,15 @@ describe('scoped-ids', () => {
describe('custom elements', () => {
it('should render expected id attribute value when its value is set to `undefined`', () => {
const elm = createElement('x-foo', { is: CustomElementIdValueUndefined });
expect(() => {
document.body.appendChild(elm);
}).toLogErrorDev(
/\[LWC error\]: Invalid id value "undefined". The id attribute must contain a non-empty string./
);
document.body.appendChild(elm);
const child = elm.shadowRoot.querySelector('x-child');
// #1769: The difference in behavior of id attribute is tracked with this issue
expect(child.getAttribute('id')).toBe('undefined');
});

it('should render the id attribute as a boolean attribute when its value is set to an empty string', () => {
const elm = createElement('x-foo', { is: CustomElementIdValueEmpty });
expect(() => {
document.body.appendChild(elm);
}).toLogErrorDev(
/\[LWC error\]: Invalid id value "". The id attribute must contain a non-empty string./
);
document.body.appendChild(elm);
const child = elm.shadowRoot.querySelector('x-child');
expect(child.getAttribute('id')).toBe('');
});
Expand All @@ -44,22 +36,14 @@ describe('scoped-ids', () => {
describe('native elements', () => {
it('should not render id attribute when its value is set to null', () => {
const elm = createElement('x-foo', { is: IdValueUndefined });
expect(() => {
document.body.appendChild(elm);
}).toLogErrorDev(
/\[LWC error\]: Invalid id value "undefined". The id attribute must contain a non-empty string./
);
document.body.appendChild(elm);
const div = elm.shadowRoot.querySelector('div');
expect(div.getAttribute('id')).toBeNull();
});

it('should render the id attribute as a boolean attribute when its value is set to an empty string', () => {
const elm = createElement('x-foo', { is: IdValueEmpty });
expect(() => {
document.body.appendChild(elm);
}).toLogErrorDev(
/\[LWC error\]: Invalid id value "". The id attribute must contain a non-empty string./
);
document.body.appendChild(elm);
const div = elm.shadowRoot.querySelector('div');
expect(div.getAttribute('id')).toBe('');
});
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
import { createElement } from 'lwc';
import { spyConsole } from 'test-utils';

import AriaStatic from 'x/ariaStatic';
import AriaDynamic from 'x/ariaDynamic';
Expand All @@ -26,27 +25,8 @@ function testAria(type, create) {
beforeEach(async () => {
elm = create();

const consoleSpy = spyConsole();
try {
document.body.appendChild(elm);
await Promise.resolve();

// empty string (`<div id="">`) is expected to log an error, but not boolean true (`<div id>`)
// due to backwards compat
if (process.env.NODE_ENV !== 'production' && type === 'empty-string') {
expect(consoleSpy.calls.error.length).toEqual(10);
for (const call of consoleSpy.calls.error) {
expect(call[0].message).toMatch(
/The id attribute must contain a non-empty string/
);
}
} else {
expect(consoleSpy.calls.error.length).toEqual(0);
}
expect(consoleSpy.calls.warn.length).toEqual(0);
} finally {
consoleSpy.reset();
}
document.body.appendChild(elm);
await Promise.resolve();
});

it('should transform the `for` attribute value', () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,9 @@ import HrefDynamic from 'x/hrefDynamic';
import HrefDangling from 'x/hrefDangling';
import HrefBooleanTrue from 'x/hrefBooleanTrue';
import HrefBooleanTrueNoId from 'x/hrefBooleanTrueNoId';
import HrefDynamicEmptyString from 'x/hrefDynamicEmptyString';
import HrefDynamicUndefined from 'x/hrefDynamicUndefined';
import HrefDynamicNull from 'x/hrefDynamicNull';

function testHref(type, create) {
describe(`${type} href attribute values`, () => {
Expand Down Expand Up @@ -96,3 +99,30 @@ describe('boolean true', () => {
});
});
});

describe('dynamic empty value', () => {
const scenarios = [
{
name: 'empty string',
Ctor: HrefDynamicEmptyString,
tagName: 'x-href-dynamic-empty-string',
},
{ name: 'undefined', Ctor: HrefDynamicUndefined, tagName: 'x-href-dynamic-undefined' },
{ name: 'null', Ctor: HrefDynamicNull, tagName: 'x-href-dynamic-null' },
];

scenarios.forEach(({ name, Ctor, tagName }) => {
describe(name, () => {
it('renders href as expected', () => {
const elm = createElement(tagName, { is: Ctor });
document.body.appendChild(elm);

const expected = name === 'empty string' ? '' : null;

expect(elm.shadowRoot.querySelector('.sanjo').getAttribute('id')).toBe(expected);
expect(elm.shadowRoot.querySelector('a').getAttribute('href')).toBe(expected);
expect(elm.shadowRoot.querySelector('area').getAttribute('href')).toBe(expected);
});
});
});
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
<template>
<div class="sanjo" id={emptyString}></div>

<div>
<a class="anchor-empty-string" href={emptyString} data-id={emptyString}>fragment url</a>
</div>

<div>
<map name="blackdot">
<area class="area-empty-string" href={emptyString} data-id={emptyString} shape="circle" coords="75,75,75" />
</map>
</div>
</template>
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
import { LightningElement } from 'lwc';

export default class HrefDynamicEmptyString extends LightningElement {
get emptyString() {
return '';
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
<template>
<div class="sanjo" id={nullo}></div>

<div>
<a class="anchor-empty-string" href={nullo} data-id={nullo}>fragment url</a>
</div>

<div>
<map name="blackdot">
<area class="area-empty-string" href={nullo} data-id={nullo} shape="circle" coords="75,75,75" />
</map>
</div>
</template>
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
import { LightningElement } from 'lwc';

export default class HrefDynamicNull extends LightningElement {
get nullo() {
return null;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
<template>
<div class="sanjo" id={undef}></div>

<div>
<a class="anchor-empty-string" href={undef} data-id={undef}>fragment url</a>
</div>

<div>
<map name="blackdot">
<area class="area-empty-string" href={undef} data-id={undef} shape="circle" coords="75,75,75" />
</map>
</div>
</template>
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
import { LightningElement } from 'lwc';

export default class HrefDynamicUndefined extends LightningElement {
get undef() {
return undefined;
}
}
1 change: 1 addition & 0 deletions packages/@lwc/template-compiler/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ const { code, warnings } = compile(`<template><h1>Hello World!</h1></template>`,
- `enableLwcSpread` (boolean, optional, `true` by default) - Deprecated. Ignored by template-compiler. `lwc:spread` is always enabled.
- `customRendererConfig` (CustomRendererConfig, optional) - specifies a configuration to use to match elements. Matched elements will get a custom renderer hook in the generated template.
- `instrumentation` (InstrumentationObject, optional) - instrumentation object to gather metrics and non-error logs for internal use. See the `@lwc/errors` package for details on the interface.
- `disableSyntheticShadowSupport` (type: `boolean`, default: `false`) - Set to true if synthetic shadow DOM support is not needed, which can result in smaller/faster output.

- Example 1: Config to match `<use>` elements under the `svg` namespace and have `href` attribute set.

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ describe('customRendererConfig normalization', () => {
},
],
},
"disableSyntheticShadowSupport": false,
"enableDynamicComponents": false,
"enableLwcSpread": true,
"enableStaticContentOptimization": true,
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
<template lwc:render-mode="light">
<!-- static values -->
<label for="foo">Click me:</label>
<input type="checkbox" id="foo">

<a href="#bar">Click to scroll</a>
<section id="bar">Scroll to me</section>

<!-- dynamic values -->
<label for={foo}>Click me:</label>
<input type="checkbox" id={foo}>

<a href={bar}>Click to scroll</a>
<section id={bar}>Scroll to me</section>
</template>
Loading

0 comments on commit 750f557

Please sign in to comment.